В некоторых сферах невозможно разрабатывать программное обеспечение, ограничиваясь только знаниями программирования. Например, медицинский софт или музыкальный, о котором пойдёт речь в этой статье. Тут нужны консультации профильных экспертов. Это может удорожить разработку. Поэтому иногда экономят на качестве кода. Попробуем показать на примере проекта MuseScore, насколько важна экспертиза качества кода, скрасив технический текст программистским и музыкальным юмором.
MuseScore — компьютерная программа, редактор нотных партитур для операционных систем Windows, macOS X и Linux. MuseScore позволяет быстро вводить ноты как с клавиатуры компьютера, так и с внешней MIDI-клавиатуры. Поддерживается импорт и экспорт данных в форматах MIDI, MusicXML, LilyPond, а также импорт файлов в форматах MusE, Capella и Band-in-a-Box. Кроме того, программа может экспортировать партитуры в файлы PDF, SVG и PNG либо в документы LilyPond для дальнейшей точной доработки.
Ранее мы уже проверяли код MuseScore в 2017 году. Тогда вдохновения хватило на целый цикл из 5 статей с обзором кода разных программ для написания музыки.
MuseScore – очень крутая музыкальная платформа. Особенно для любителей только найти ноты популярной мелодии. Кроме десктопного приложения, можно пользоваться сайтом или мобильным приложением. Сейчас загрузка готовых нот стала платной по подписке, но это естественный этап в развитии успешного сервиса. Будем надеяться, часть зарабатываемых средств в будущем направится на повышение качества кода. Почему этому пора уделить внимание, читайте далее.
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<std::pair<ReducedFraction, TDuration> >& 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 ;С другой стороны, отрицательное значение в качестве позиции выглядит странно.
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 была добавлена позже и не туда – до проверки указателя.
Это были самые очевидные срабатывания такой диагностики, т.к. проблемные места находились друг от друга на расстоянии нескольких строк. Другие места, в которых стоит разобраться, приведу списком:
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;
....
}Анализатор обнаружил несколько опасных мест, когда в поле класса сохраняется указатель на локальный объект, созданный в одной из функций. В дальнейшем такой указатель может вести на мусорные данные в памяти.
Все такие места нашлись в одном файле:
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.
Весь список похожих мест с перезаписью значений переменных:
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. Скорее всего, такие дефолтные значения не планировалось получать.
В целом, качеству проектов с открытым исходным кодом уделяется не очень много внимания. Иначе мы бы не сделали столько обзоров ошибок из разных проектов. Ещё одна проблема, которая напрямую портит качество кода, – это кочевание ошибок из проекта в проект. Самый известный случай на нашей памяти – это код игрового движка 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<T,K>(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<T,K>& 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 и тоже обнаружили значительное количество дефектов кода.
Среди наших клиентов можно найти много игровых студий, банков и крупных IT-гигантов, а вот крупных представителей музыкальной индустрии нет. Надеюсь, представители этих компаний вдохновятся статьей и хотя бы попробуют триал на своих проектах.