On Mon, 3 Dec 2018, at 07:14, Pedro Lopez-Cabanillas wrote:
> More like this one, here:
> https://www.viva64.com/en/b/0536/
That's extremely interesting, thanks for the pointer!
I wonder why the author took the time to write such a long report about a
project but not to tell the project's developers about it. (Or did they, and I
just didn't see it myself?) Thought they were doing a kindness perhaps.
Well, there are horrors in there, but there are also many things that don't
result in actual bugs. Though you might not want to take my word for it, since
I am to blame for many of these in the first place.
I'll go through the reports in order, and mark them like so:
[N] - Not a bug at all (there will only be one of these!)
[S] - Severe - anything that seems to be genuinely user-visible
[E] - Easy to fix without risk of breaking anything else, and so worth fixing,
but with no user-visible consequences as far as I can see
[R] - Review required - should not be changed without understanding it better -
can't tell what severity or what the correct fix is
I haven't always cross-checked whether they have been fixed already.
[E] V560 A part of conditional expression is always false: singleStaff.
NotationScene.cpp 1707
Looks like my code, probably a cut-and-paste error, doesn't actually lead to
any unnecessary iterations as the author suggests it might. This is a kind of
thing I've always been rather lax about - an unnecessary test that got
forgotten because it didn't make any difference in the output.
[R] V547 Expression '!beamedSomething' is always true.
SegmentNotationHelper.cpp 1405
Looks like me like a genuine mistake caused by trying to deal with a massively
over-ambitious piece of logic in one place. I can imagine spending ages late at
night trying to figure out why changes in that big conditional didn't seem to
make any difference, and then thinking "ah stuff it" and eventually going to
bed.
[R] V547 Expression 'i > 5' is always false. SegmentParameterBox.cpp 323
Likely me as well. This is in the construction of a combobox in the GUI, so I
probably stopped worrying about it once the apparently-correct result appeared
on screen. Same slackness as V560 above.
[R] V547 Expression 'adjustedOctave < 8' is always false. NotePixmapFactory.cpp
1920
Also me. I don't think this one makes any difference to the result either, but
reading this sort of code does my head in a bit now and I clearly shouldn't
have written it that way.
[E] V547 Expression '""' is always true. LilyPondOptionsDialog.cpp 64
Nice little typo there that I guess nobody noticed the result of.
[E] V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected.
There is a probability of logical error presence. Check lines: 223, 239.
IntervalDialog.cpp 223
This is interesting. I guess this code is quite tricky to reason about (it
certainly feels beyond my own pay grade). I'm guessing the spurious "== 0" case
at the end is a left-over bit cut-and-pasted from a very similar block of code
just above. Would benefit from unit tests, but again probably not a
user-visible bug. (I suppose this kind of logic is also tricky for translators.)
[E] V773 The function was exited without releasing the 'testFile' pointer. A
memory leak is possible. RIFFAudioFile.cpp 561 (and the following two)
I think the first two here are Richard's and the third is mine, and they all
speak to a serious underlying confusion about how error reporting is done with
std::iostreams. I think at this point we were still in cargo-culting mode for
std::iostream code and would have been happier avoiding it. This code is a
symptom of trying to get a job done without really wanting to know how your
tools work.
[E] V601 The integer type is implicitly cast to the char type. MidiEvent.cpp 181
This is the one this thread was about - it's the first one that I would agree
is a real facepalm. I don't *think* it's mine, but it's not impossible that I
would have done this since at the time in my day job I was mostly writing
languages that would do this conversion. And isn't it amazing that this isn't
an error in C++?
[R] V674 The '0.1' literal of the 'double' type is compared to a value of the
'int' type. Consider inspecting the 'm_connectingLineLength > 0.1' expression.
StaffLayout.cpp 1028
Fascinating error and I agree that this speaks of some nasty inconsistency of
intention on the programmer's part (probably mine again).
[S] V601 The string literal is implicitly cast to the bool type. FileSource.cpp
902
Looks like a classic typo (cut-and-paste, or else changing the types throughout
and missing one) and a definite bug.
[S] V783 Dereferencing of the invalid iterator 'i' might take place.
IconStackedWidget.cpp 126
Not my code, but I wince in sympathy with the programmer.
[S] V783 Dereferencing of the invalid iterator 'beatTimeTs.end()' might take
place. CreateTempoMapFromSegmentCommand.cpp 119
My code I think, and a real mistake. The article guesses at a fix, but it's not
correct - the intention was to subtract 1 from the final timeT, not only from
the iterator.
[S] V783 Dereferencing of the invalid iterator 'm_segments.end()' might take
place. StaffHeader.cpp 250
Not mine, but also a real mistake - a misunderstanding about iterators I think.
[R] V1004 The 'track' pointer was used unsafely after it was verified against
nullptr. Check lines: 319, 329. MatrixView.cpp 329
This one is quite a deep problem, in that we never had a coherent plan for how
to deal with error handling across API boundaries like this. In principle,
Composition::getTrackById shouldn't ever return null (in fact this sparks a
"this is probably a BUG" message that is sometimes seen on stderr) - it should
use an exception. In practice that doesn't happen, and if it were changed now
so as to work that way, other existing code would break. But this conceptual
inconsistency (the idea that an invalid id is a logical error already before
the call is ever made, even though the code being called doesn't treat it that
way) leads to writing code that treats this call as unable to fail.
[R] There follows a long list of dodgy nullptr checks - this kind of thing is
problematic throughout RG and is really typical of a sort of nasty old-school
C++ - I haven't reviewed them all but there's a mixture of unnecessary
defensive checks (because the pointer logically can't be null at that point)
and real hazards.
[S] V670 The uninitialized class member 'm_intervals' is used to initialize the
'm_size' member. Remember that members are initialized in the order of their
declarations inside a class. Tuning.cpp 394
First one to be attributable to Michael, I think. The author mentions that this
is not common - I thought that compilers warned about it, which would explain
why it's not common but not why this example persisted.
[E] V557 Array overrun is possible. The value of 'submaster' index could reach
64. SequencerDataBlock.cpp 325
My fault, plain stupidity.
[R] V612 An unconditional 'break' within a loop. Fingering.cpp 83
First one from Guillaume I think. I see Ted has already looked at this one and
marked it as needing review.
[E] V746 Object slicing. An exception should be caught by reference rather than
by value. MupExporter.cpp 197
I'm not even going to look at who wrote this particular example because I know
that I've done this all over the place in this and other projects - just
writing down what seemed like the obvious code without thinking through what it
actually meant. I imagine I'm not the only one. Switching to catching by
reference everywhere is presumably safe enough.
[N] V523 The 'then' statement is equivalent to the 'else' statement.
HydrogenXMLHandler.cpp 476
In contrast to all of the above, I think this code (from Thorsten Alteholz) was
intended to be that way - it's written in a way that provides a cue to future
developers about what to do when a new release is added.
While I'm here I'll see about fixing a few of these.
Chris
_______________________________________________
Rosegarden-devel mailing list
Rosegarden-devel@lists.sourceforge.net - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
