Re: [Canorus-devel] Layoutengine: Possible Bug with Fingering

[Reinhard K. <su...@gm...>]

Hi Matevz,

Simple answer: I prefer clean code ;)

Meaning if I have difficulties understanding the code with too many || and && you can introduce f.e. (temporary) variables/members or even methods/classes. 

Also afterwards it's much easier to maintain the code, especially if you only work on it once a month or so.

This could even optimizes the code, especially when fingering, repeat or similar marks do not change.

Regards,

Reinhard

-------- Original-Nachricht --------
> Datum: Tue, 04 Sep 2012 01:24:54 +0200
> Von: "Matevž Jekovec" <ma...@je...>
> An: can...@li...
> Betreff: Re: [Canorus-devel] Layoutengine: Possible Bug with Fingering

> Hi Reinhard.
> 
> I spotted the origin of the repeat sign crash. You put the paranthesis 
> in R1307 around the wrong condition. It's
> 
> mark->markType()==CAMark::Fingering && (LHeel || LToe)
> 
> and not
> 
> (mark->markType()==CAMark::Fingering && LHeel) || LToe)
> 
> Now we don't need dynamic_cast anymore ;)
> 
> Anyway, I'm not sure what to do about the paranthesis style - it's a 
> matter of taste. Personally I hate paranthesis because they only clutter 
> code (remember python's if statements!). And C++ operator precedence is 
> very clear 
> (http://en.cppreference.com/w/cpp/language/operator_precedence). The 
> original R1306 was of that style. However, this produces warnings which 
> I don't really understand why. Any thoughts on this?
> 
> 
> Regards.
> -Matevž
> 
> Dne 02. 09. 2012 17:45, piše Reinhard Katzmann:
> > Hi,
> >
> > I fixed the crash for now using solution b) and (most important)
> > I made an "instanceof" check (dynamic cast) for fingering.
> >
> > I seems that for some reason the repeat mark was recognized
> > as fingering but the instance of course is no fingering hence
> > the fingerlist() call can't work.
> >
> > Regards,
> >
> > Reinhard
> >
> > Am Sun, 2 Sep 2012 17:07:56 +0200
> > gab Reinhard Katzmann <su...@we...> folgendes zu Papier:
> >
> >> Hi,
> >>
> >> When trying to find a workaround on the crash with fingering
> >> I think I found a logical bug:
> >>
> >> // This is from an older version without my changes from Large Warn
> Fixes:
> >> if (   mark->markType()==CAMark::Pedal ||         // 1: Mark -> Pedal
> >>       ( mark->markType()==CAMark::Fingering &&     // 2: Mark ->
> Fingering
> >> 	(static_cast<CAFingering*>(mark)->fingerList()[0]==CAFingering::LHeel
> >> 		||
> >> 	 static_cast<CAFingering*>(mark)->fingerList()[0]==CAFingering::LToe
> >> 		))
> >>                                                    // 3: Muselement ->
> Note
> >>      || elt->musElementType()==CAMusElement::Note &&
> >> 	static_cast<CANote*>(elt)->actualSlurDirection()==CASlur::SlurDown
> >> 		&&
> >>          (mark->markType()==CAMark::Text ||
> >> 		mark->markType()==CAMark::Fermata ||
> >> 		mark->markType()==CAMark::Articulation ||
> >> 		mark->markType()==CAMark::Fingering && // Why? it can't be
> >> 		static_cast<CAFingering*>(mark)->fingerList()[0]!=CAFingering::LHeel
> >> 		&&
> >> 		static_cast<CAFingering*>(mark)->fingerList()[0]!=CAFingering::LToe
> >> 		))  {
> >>
> >> Example:
> >> 1) Check mark (no pedal)
> >> 2) Check different mark (no fingering)
> >> 3) Check note muselement (is a note muselement)
> >>       Check Slur Direction (is down)
> >>       Check Mark Type
> >>         -> How can it ever be fingering ?
> >>
> >> Two solutions to the problem:
> >> a) check for muselement note (easy but not elegant)
> >> b) remove check for fingering in note element (elegant)
> >>
> >> Regards,
> >>
> >> Reinhard
> >
> >
> 
> 
> ------------------------------------------------------------------------------
> Live Security Virtual Conference
> Exclusive live event will cover all the ways today's security and 
> threat landscape has changed and how IT managers can respond. Discussions 
> will include endpoint security, mobile security and the latest in malware 
> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
> _______________________________________________
> Canorus-devel mailing list
> Can...@li...
> https://lists.sourceforge.net/lists/listinfo/canorus-devel