Three coding bugs identified by static analysis

[shoogle]

Issue type

Other type of issue

Bug description

Coding bugs identified by this video https://www.youtube.com/watch?v=SAVbpFTj81I

The video is a year old so some of the errors don't exist anymore.

Steps to reproduce

These are the ones that are still active:

Bug at 01:11

i-i is always zero https://github.com/musescore/MuseScore/blob/fae324d86b7f03e9e2e16b11548be819b7a04486/src/engraving/libmscore/textbase.cpp#L1888

Bug at 01:17

Functions are identical https://github.com/musescore/MuseScore/blob/fae324d86b7f03e9e2e16b11548be819b7a04486/src/engraving/libmscore/rest.cpp#L735-L753

Bug at 2m33

destinationMeasure is dereferrenced with -> in multiple places without first checking for nullptr. Same goes for a few other variables in that function. https://github.com/musescore/MuseScore/blob/fae324d86b7f03e9e2e16b11548be819b7a04486/src/engraving/libmscore/score.cpp#L4668-L4713

And there are also some negated assignments within the if conditions if (!(foo = foo->bar)) which aren't exactly easy to follow. Specifically for these lines, destinationMeasure is guaranteed to be nullptr when it is dereferenced on the second line because of the condition on the line above.

https://github.com/musescore/MuseScore/blob/fae324d86b7f03e9e2e16b11548be819b7a04486/src/engraving/libmscore/score.cpp#L4704-L4705

Screenshots/Screen recordings

No response

MuseScore Version

Latest code on master

Regression

No.

Operating system

All

Additional context

No response

[AntonioBL]

~~Even if not strictly found by a static analyzer, there is also a compilation warning at line 2117 of src/engraving/libmscore/note.cpp: warning: '?:' using integer constants in boolean context, the expression will always evaluate to 'true' If I understood correctly, && has precedence over ?, so the condition can be basically reduced to: if ( condition1 ? 2 : -2 ), i.e. if(numeric_const_different_form_0) always true~~ (Edit: already fixed in master)

[willkell]

Hello, can I take a look at this?

[bkunda]

@willkell thanks for volunteering for this one! Are you still working on it?

[BrownianNotion]

Happy to pick this up if still relevant. I'm still quite new so I will likely return with questions.

[BrownianNotion]

Looks like the first bug has now been fixed, but the other two remain. Not sure that the methods Rest::upLine and Rest::downLine are actually being used anywhere though.

[Jojo-Schmitz]

They are used, at least in https://github.com/musescore/MuseScore/blob/b38394fb5cf4bfdcbbf7b32248783889ffc5db85/src/engraving/dom/chordrest.h#L90-L91

[BrownianNotion]

They are used, at least in

https://github.com/musescore/MuseScore/blob/b38394fb5cf4bfdcbbf7b32248783889ffc5db85/src/engraving/dom/chordrest.h#L90-L91

But this is ChordRest::upLine? This method is overridden by Rest::upLine upon inheriting from the ChordRest class.

[Jojo-Schmitz]

That's why it is used

[BrownianNotion]

I see what you're saying now, sorry for misunderstanding :)

[wizofaus]

Can this be assigned to @BrownianNotion so it's clear it's already being looked at?

[BrownianNotion]

Thanks @wizofaus. Haven't had a chance to take a good look yet, aiming to start this week.