In some areas, it is impossible to develop software with just programming knowledge. For example, medical software or music software, which will be discussed in this article. Here we need advice from specialized experts. This can make development more expensive. Therefore, sometimes they save on the quality of the code. Let's try to show by the example of the MuseScore project how important the code quality examination is, brightening up the technical text with programming and musical humor.
Introduction
MuseScore — , Windows, Mac OS X Linux. MuseScore , MIDI-. MIDI, MusicXML, LilyPond, MusE, Capella Band-in-a-Box. , PDF, SVG PNG LilyPond .
MuseScore – . . , . , . , . , .
Copy-paste code
V501 There are identical sub-expressions to the left and to the right of the '==' operator: desiredLen == desiredLen importmidi_simplify.cpp 44
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;
}
- . - desiredLen . , :
return desiredLen == sum;
V501 There are identical sub-expressions to the left and to the right of the '-' operator: i - i textbase.cpp 1986
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);
}
....
}
layout , , , .
V523 The 'then' statement is equivalent to the 'else' statement. bsp.cpp 194
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);
}
....
}
– . . . , , .
V524 It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function. rest.cpp 718
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);
}
upLine downLine , . , - .
V778 Two similar code fragments were found. Perhaps, this is a typo and 'description' variable should be used instead of 'name'. instrumentsreader.cpp 407
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();
}
....
}
instrument.name * instrument.description* , . "name" "description" – . , , longNames.
, .
V1063 The modulo by 1 operation is meaningless. The result will always be zero. lyrics.h 85
class Lyrics final : public TextBase
{
....
bool isEven() const { return _no % 1; }
....
}
. isEven true, , – false (). , , - 1, 2 false. .. .
V1065 Expression can be simplified, check '1' and similar operands. scorediff.cpp 444
QString MscxModeDiff::getOuterLines(const QString& str, int lines, bool start)
{
lines = qAbs(lines);
const int secIdxStart = start ? 0 : (-1 - (lines - 1));
....
}
, . :
const int secIdxStart = start ? 0 : -lines ;
, .
: C++
V522 Dereferencing of the null pointer 'family' might take place. instrtemplate.cpp 356
void InstrumentTemplate::write(XmlWriter& xml) const
{
....
if (!family) {
xml.tag("family", family->id);
}
xml.etag();
}
"family" - , .
V522 Dereferencing of the null pointer 'destinationMeasure' might take place. score.cpp 4279
ChordRest* Score::cmdNextPrevSystem(ChordRest* cr, bool next)
{
....
auto destinationMeasure = currentSystem->firstMeasure();
....
if (!(destinationMeasure = destinationMeasure->prevMeasure())) {
if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) {
return cr;
}
}
....
}
, . destinationMeasure .
V595 The 'fd' pointer was utilized before it was verified against nullptr. Check lines: 5365, 5366. edit.cpp 5365
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());
}
....
}
Fret Diagram ( FretBoard) , , . . , fd , . , . .. , , , .
V595 The 'startSegment' pointer was utilized before it was verified against nullptr. Check lines: 129, 131. notationselectionrange.cpp 129
Ms::Segment* NotationSelectionRange::rangeStartSegment() const
{
Ms::Segment* startSegment = score()->selection().startSegment();
startSegment->measure()->firstEnabled(); // <=
if (!startSegment) { // <=
return nullptr;
}
if (!startSegment->enabled()) {
startSegment = startSegment->next1MMenabled();
}
....
}
, . , startSegment – .
, .. . , , :
V595 The 'note' pointer was utilized before it was verified against nullptr. Check lines: 5932, 5941. importmxmlpass2.cpp 5932
V595 The 'ed' pointer was utilized before it was verified against nullptr. Check lines: 599, 608. textedit.cpp 599
V595 The 's' pointer was utilized before it was verified against nullptr. Check lines: 139, 143. elements.cpp 139
V774 The 'slur' pointer was used after the memory was released. importgtp-gp6.cpp 2592
void GuitarPro6::readGpif(QByteArray* data)
{
....
if (c) {
slur->setTick2(c->tick());
score->addElement(slur);
legatos[slur->track()] = 0;
} else {
delete slur;
legatos[slur->track()] = 0;
}
....
}
, . . , MuseScore . . . , .
V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 4439, 4440. exportxml.cpp 4439
virtual Fraction tick() const override { return _tick; }
void ExportMusicXml::hairpin(....)
{
....
if (hp->tick() != tick) {
writeHairpinText(_xml, hp, hp->tick() == tick);
}
....
}
, writeHairpinText , 3- false.
tick :
virtual Fraction tick() const override { return _tick; }
.. - , , .
V763 Parameter 'y' is always rewritten in function body before being used. tremolo.cpp 287
void Tremolo::layoutOneNoteTremolo(qreal x, qreal y, qreal spatium)
{
bool up = chord()->up();
int line = up ? chord()->upLine() : chord()->downLine();
....
qreal yLine = line + t;
....
y = yLine * .5 * spatium;
setPos(x, y);
}
. , - . y.
V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4391
class BasicParse
{
....
protected:
StreamHandle* m_handle;
....
}
bool OvscParse::parse()
{
Block* dataBlock = m_chunk->getDataBlock();
unsigned int blockSize = m_chunk->getSizeBlock()->toSize();
StreamHandle handle(dataBlock->data(), blockSize);
Block placeHolder;
m_handle = &handle;
....
}
, , . .
:
V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4483
V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4930
V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 9291
V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 9507
V519 The 'savedExtension.status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 349, 352. extensionsservice.cpp 352
void ExtensionsService::th_refreshExtensions()
{
....
if (savedExtension.version < extension.version) {
savedExtension.status = ExtensionStatus::NeedUpdate;
}
savedExtension.status = ExtensionStatus::Installed;
....
}
, - - : Installed.
:
V519 The 'lyrNote' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 962, 972. importgtp-gp6.cpp 972
V519 The '_crossMeasure' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2545, 2550. chord.cpp 2550
V519 The 'bt' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 417, 418. chordrest.cpp 418
V612 An unconditional 'return' within a loop. noteinputbarmodel.cpp 371
int NoteInputBarModel::resolveCurrentVoiceIndex() const
{
....
for (const Element* element: selection()->elements()) {
return element->voice();
}
....
}
: "?".
V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. instrumentstypes.h 135
static constexpr int MAX_STAVES = 4;
enum class BracketType : signed char {
NORMAL, BRACE, SQUARE, LINE, NO_BRACKET = -1
};
struct Instrument
{
....
BracketType bracket[MAX_STAVES] = { BracketType::NO_BRACKET };
....
}
, bracket NO_BRACKET. - -1. , – 0. NORMAL, NO_BRACKET. , .
Open Source
, . . , , – . – Amazon Lumberyard, CryEngine . .
MuseScore . intervaltree. :
V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. IntervalTree.h 70
IntervalTree(const intervalTree& other) {
center = other.center;
intervals = other.intervals;
if (other.left) {
left = (intervalTree*) malloc(sizeof(intervalTree)); // <=
*left = *other.left;
} else {
left = NULL;
}
if (other.right) {
right = new intervalTree();
*right = *other.right;
} else {
right = NULL;
}
}
IntervalTree& operator=(const intervalTree& other) {
center = other.center;
intervals = other.intervals;
if (other.left) {
left = new intervalTree(); // <=
*left = *other.left;
} else {
left = NULL;
}
if (other.right) {
right = new intervalTree(); // <=
*right = *other.right;
} else {
right = NULL;
}
return *this;
}
malloc . new. , , C++ - new, .. intervalTree .
, , 2 . . .
?
V523 The 'then' statement is equivalent to the 'else' statement. bsp.cpp 194
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);
}
....
}
QtBase :
QString QGraphicsSceneBspTree::debug(int index) const
{
const Node *node = &nodes.at(index);
QString tmp;
if (node->type == Node::Leaf) {
QRectF rect = rectForIndex(index);
if (!leaves[node->leafIndex].isEmpty()) {
tmp += QString::fromLatin1("[%1, %2, %3, %4] contains %5 items\n")
.arg(rect.left()).arg(rect.top())
.arg(rect.width()).arg(rect.height())
.arg(leaves[node->leafIndex].size());
}
} else {
if (node->type == Node::Horizontal) {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
} else {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
}
}
return tmp;
}
MuseScore, QtBase.
. . - . PVS-Studio . Steinberg SDK Steinberg Media Technologies GmbH .
, : Svyatoslav Razmyslov. Short-lived Music or MuseScore Code Analysis.