{"id":400796,"date":"2024-06-29T15:28:26","date_gmt":"2024-06-29T15:28:26","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=400796"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=400796","title":{"rendered":"Short-lived Music or MuseScore Code Analysis<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n

Having only programming background, it is impossible to develop software in some areas. Take the difficulties of medical software development as an example. The same is with music software, which will be discussed in this article. Here you need an advice of subject matter experts. However, it’s more expensive for software development. That is why developers sometimes save on code quality. The example of the MuseScore project check, described in the article, will show the importance of code quality expertise. Hopefully, programming and musical humor will brighten up the technical text.<\/p>\n

<\/figcaption><\/figure>\n

Introduction<\/h3>\n

MuseScore<\/a> is a computer program, a scorewriter for Windows, Mac OS X, and Linux operating systems. MuseScore allows you to quickly enter notes both with the computer keyboard and with an external MIDI keyboard. The scorewriter can import and export MIDI, MusicXML, LilyPond formats. It can also import MusE, Capella, and Band-in-a-Box. In addition, the program can export the scores to PDF, SVG, and PNG files, and to LilyPond for further fine-tuning.<\/p>\n

Previously, we checked<\/a> the MuseScore code in 2017. It inspired us to write a series of 5 articles. There we reviewed the code of different programs for writing music.<\/p>\n

MuseScore is a really cool music platform. Fans of just finding popular melody notes will highly praise the program. Besides the desktop application, you can use the website or the mobile app. The download of ready-made notes has now become paid by subscription. However, it is usual for successful service development. Let’s hope, that the developers will allocate some of the earned money to improve code quality. Read on to find out why it’s time to pay attention to this.<\/p>\n

Copy-paste code<\/h3>\n
<\/figcaption><\/figure>\n

V501<\/a> There are identical sub-expressions to the left and to the right of the ‘==’ operator: desiredLen == desiredLen importmidi_simplify.cpp 44<\/p>\n

bool areDurationsEqual(   const QList >& durations,   const ReducedFraction& desiredLen) {   ReducedFraction sum(0, 1);   for (const auto& d: durations) {     sum += ReducedFraction(d.second.fraction()) \/ d.first;   }    return desiredLen == desiredLen; } <\/code><\/pre>\n
The comparison function for durations of notes (or some such) returns an incorrect result. All because of the copied desiredLen<\/em> variable at the very end of the function. The correct code is most likely to look like this:<\/p>\n
return desiredLen == sum; <\/code><\/pre>\n
V501<\/a> There are identical sub-expressions to the left and to the right of the ‘-‘ operator: i — i textbase.cpp 1986<\/p>\n
void TextBase::layout1() {   ....   for (int i = 0; i < rows(); ++i) {     TextBlock* t = &_layout[i];     t->layout(this);     const QRectF* r = &t->boundingRect();      if (r->height() == 0) {       r = &_layout[i - i].boundingRect();    \/\/ <=     }     y += t->lineSpacing();     t->setY(y);     bb |= r->translated(0.0, y);   }   .... } <\/code><\/pre>\n
The null element is always taken from the layout<\/em> array because an error has slipped into the expression that calculates the index.<\/p>\n
V523<\/a> The ‘then’ statement is equivalent to the ‘else’ statement. bsp.cpp 194<\/p>\n
QString BspTree::debug(int index) const {   ....   if (node->type == Node::Type::HORIZONTAL) {     tmp += debug(firstChildIndex(index));     tmp += debug(firstChildIndex(index) + 1);   } else {     tmp += debug(firstChildIndex(index));     tmp += debug(firstChildIndex(index) + 1);   }   .... } <\/code><\/pre>\n
Code debugging is already a consequence of an earlier error in the code. Only errors in the debugging code can make the situation worse. Here the code of the two branches of the conditional operator is absolutely identical. No prizes for guessing that the code was copied to speed up the development. However, someone forgot to make changes to the second copy of the code.<\/p>\n
V524<\/a> It is odd that the body of ‘downLine’ function is fully equivalent to the body of ‘upLine’ function. rest.cpp 718<\/p>\n
int Rest::upLine() const {     qreal _spatium = spatium();     return lrint((pos().y() + bbox().top() + _spatium) * 2 \/ _spatium); }  int Rest::downLine() const {     qreal _spatium = spatium();     return lrint((pos().y() + bbox().top() + _spatium) * 2 \/ _spatium); } <\/code><\/pre>\n
The functions’ names upLine<\/em> and downLine<\/em> reflect the opposite meaning. Though, this is not supported by the implementation of these functions. Most likely, there is another error caused by copying the code.<\/p>\n
V778<\/a> Two similar code fragments were found. Perhaps, this is a typo and ‘description’ variable should be used instead of ‘name’. instrumentsreader.cpp 407<\/p>\n
void InstrumentsReader::fillByDeffault(Instrument& instrument) const {   ....   if (instrument.name.isEmpty() && !instrument.longNames.isEmpty()) {       instrument.name = instrument.longNames[0].name();   }   if (instrument.description.isEmpty() && !instrument.longNames.isEmpty()) {       instrument.description = instrument.longNames[0].name();   }   .... } <\/code><\/pre>\n
Fields instrument.name<\/em> and* instrument.description* are initialized with the same values. This makes the code suspicious. The names «name» and «description» are entities with quite different meanings. The index used to access the longNames<\/em> array is most likely to differ here.<\/p>\n
The new diagnostics’ debut<\/h3>\n
<\/figcaption><\/figure>\n
Since the last review of this project, we have made some new diagnostics. They’ve helped us to find even more interesting errors.<\/p>\n
V1063<\/a> The modulo by 1 operation is meaningless. The result will always be zero. lyrics.h 85<\/p>\n
class Lyrics final : public TextBase {   ....   bool isEven() const { return _no % 1; }   .... } <\/code><\/pre>\n
One of the new diagnostics found a very amusing error. The isEven<\/em> function must return true<\/em> if the number is even, otherwise, it must return false<\/em> (odd). In fact, due to taking the remainder of 1, not 2, the function always returns the *false *value. That is, all numbers are considered odd.<\/p>\n
V1065<\/a> Expression can be simplified, check ‘1’ and similar operands. scorediff.cpp 444<\/p>\n
QString MscxModeDiff::getOuterLines(const QString& str, int lines, bool start) {     lines = qAbs(lines);     const int secIdxStart = start ? 0 : (-1 - (lines - 1));     .... } <\/code><\/pre>\n
Perhaps, this is not an error. However, we can greatly simplify the code. So, here’s what it looks like:<\/p>\n
const int secIdxStart = start ? 0 : -lines ; <\/code><\/pre>\n
On the other hand, the negative value as a position looks strange. <\/p>\n
Pointers in C++: a timeless classic<\/h3>\n
<\/figcaption><\/figure>\n
V522<\/a> Dereferencing of the null pointer ‘family’ might take place. instrtemplate.cpp 356<\/p>\n
void InstrumentTemplate::write(XmlWriter& xml) const {   ....   if (!family) {     xml.tag(\"family\", family->id);   }   xml.etag(); } <\/code><\/pre>\n
Inasmuch as the extra negation was written in the conditional expression, the added «family» tag can spell disaster.<\/p>\n
V522<\/a> Dereferencing of the null pointer ‘destinationMeasure’ might take place. score.cpp 4279<\/p>\n
ChordRest* Score::cmdNextPrevSystem(ChordRest* cr, bool next) {   ....   auto destinationMeasure = currentSystem->firstMeasure();   ....   if (!(destinationMeasure = destinationMeasure->prevMeasure())) {     if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) {         return cr;     }   }   .... } <\/code><\/pre>\n
This is a similar but less obvious situation. Here access to the destinationMeasure<\/em> pointer in a nested conditional expression takes place. It’s dereferencing the null pointer.<\/p>\n
V595<\/a> The ‘fd’ pointer was utilized before it was verified against nullptr. Check lines: 5365, 5366. edit.cpp 5365<\/p>\n
void Score::undoAddElement(Element* element) {   ....   FretDiagram* fd = toFretDiagram(ne);   Harmony* fdHarmony = fd->harmony();   if (fd) {     fdHarmony->setScore(score);     fdHarmony->setSelected(false);     fdHarmony->setTrack(staffIdx * VOICES + element->voice());   }   .... } <\/code><\/pre>\n
Fret Diagram (or FretBoard) is also used to record melodies \u2013 for instance, by guitarists. However, they are a bit out of luck. The error here is that the fd<\/em> pointer is dereferenced before its validity is checked. The name of the function suggests that it happens when the addition of an element gets canceled. That is, the rollback of some changes in the notes can accidentally break the program. Thus, you’ll probably lose the notes.<\/p>\n
V595<\/a> The ‘startSegment’ pointer was utilized before it was verified against nullptr. Check lines: 129, 131. notationselectionrange.cpp 129<\/p>\n
Ms::Segment* NotationSelectionRange::rangeStartSegment() const {   Ms::Segment* startSegment = score()->selection().startSegment();    startSegment->measure()->firstEnabled();  \/\/ <=    if (!startSegment) {                      \/\/ <=     return nullptr;   }    if (!startSegment->enabled()) {     startSegment = startSegment->next1MMenabled();   }   .... } <\/code><\/pre>\n
Unlike the previous code snippet, it seems to be failed refactoring. Most likely, the line dereferencing the startSegment<\/em> pointer was added later. Moreover, it was displaced. It stands before the pointer validation.<\/p>\n
These were the most obvious warnings from this diagnostic. They were several lines apart from each other. Here’s a list of some other places that are worth viewing:<\/p>\n