Программирование — занятие творческое, поэтому среди разработчиков встречается много талантливых людей, имеющих своеобразное хобби. Вопреки распространённому мнению, это не всегда программирование (ну или не только оно :D). На основе своего увлечения записью/обработкой музыки и профессиональной деятельности, я решил проверить качество кода популярных музыкальных программ с открытым исходным кодом. Первой для обзора выбрана программа для редактирования нот — MuseScore. Запасайтесь попкорном… серьёзных багов будет много!

Введение


MuseScore — компьютерная программа, редактор нотных партитур для операционных систем Windows, Mac OS X и Linux. MuseScore позволяет быстро вводить ноты как с клавиатуры компьютера, так и с внешней MIDI-клавиатуры. Поддерживается импорт и экспорт данных в форматах MIDI, MusicXML, LilyPond, а также импорт файлов в форматах MusE, Capella и Band-in-a-Box. Кроме того, программа может экспортировать партитуры в файлы PDF, SVG и PNG, либо в документы LilyPond для дальнейшей точной доработки партитуры.

PVS-Studio — это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в среде Windows и Linux.

Проблемы с индексацией массивов




V557 Array overrun is possible. The value of 'cidx' index could reach 4. staff.cpp 1029

ClefTypeList clefTypes[MAX_STAVES];
int staffLines[MAX_STAVES];
BracketType bracket[MAX_STAVES];
int bracketSpan[MAX_STAVES];
int barlineSpan[MAX_STAVES];
bool smallStaff[MAX_STAVES];

void Staff::init(...., const StaffType* staffType, int cidx)
{
  if (cidx > MAX_STAVES) { // <=
    setSmall(0, false);
  }
  else {
    setSmall(0,       t->smallStaff[cidx]);
    setBracketType(0, t->bracket[cidx]);
    setBracketSpan(0, t->bracketSpan[cidx]);
    setBarLineSpan(t->barlineSpan[cidx]);
  }
  ....
}

Автор этого фрагмента кода допустил серьёзную ошибку, сравнивая индекс с максимальным размером массива. По этой причине стал возможен выход за пределы границ сразу четырёх массивов.

Исправленное условие проверки индекса:

if (cidx >= MAX_STAVES) {
  setSmall(0, false);
}

V557 Array overrun is possible. The value of 'i' index could reach 59. inspectorAmbitus.cpp 70

class NoteHead : public Symbol {
  ....
public:
  enum class Group : signed char {
    HEAD_NORMAL = 0,
    HEAD_CROSS,
    HEAD_PLUS,
    ....
    HEAD_GROUPS,              // <= 59
    HEAD_INVALID = -1
    };
  ....
}

InspectorAmbitus::InspectorAmbitus(QWidget* parent)
   : InspectorElementBase(parent)
{
  r.setupUi(addWidget());
  s.setupUi(addWidget());

  static const NoteHead::Group heads[] = {
    NoteHead::Group::HEAD_NORMAL,
    NoteHead::Group::HEAD_CROSS,
    NoteHead::Group::HEAD_DIAMOND,
    NoteHead::Group::HEAD_TRIANGLE_DOWN,
    NoteHead::Group::HEAD_SLASH,
    NoteHead::Group::HEAD_XCIRCLE,
    NoteHead::Group::HEAD_DO,
    NoteHead::Group::HEAD_RE,
    NoteHead::Group::HEAD_MI,
    NoteHead::Group::HEAD_FA,
    NoteHead::Group::HEAD_SOL,
    NoteHead::Group::HEAD_LA,
    NoteHead::Group::HEAD_TI,
    NoteHead::Group::HEAD_BREVIS_ALT
    };
  ....
  for (int i = 0; i < int(NoteHead::Group::HEAD_GROUPS); ++i)
    r.noteHeadGroup->setItemData(i, int(heads[i]));//out of bound
  ....
}

Вместо того, чтобы посчитать количество элементов массива, которые обходятся в цикле, здесь воспользовались константой, которая почти в четыре раза больше этого количества. В цикле происходит гарантированный выход за границу массива.

V501 There are identical sub-expressions to the left and to the right of the '-' operator: i — i text.cpp 1429

void Text::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);
  }
  ....
}

Значение индекса [i — i], в данном случае, всегда будет равно нулю. Возможно, тут ошибка и хотели, к примеру, обращаться к предыдущему элементу массива.

Утечки памяти



С помощью статического анализа тоже можно находить утечки памяти и PVS-Studio это делает. Да, статические анализаторы в плане поиска утечек памяти более слабы, чем динамические, но всё равно они могут найти много интересного.

В незнакомом проекте мне сложно проверить достоверность всех найденных предупреждений, но в некоторых местах мне удалось убедиться в наличии ошибки.

V773 Visibility scope of the 'beam' pointer was exited without releasing the memory. A memory leak is possible. read114.cpp 2334

Score::FileError MasterScore::read114(XmlReader& e)
{
  ....
  else if (tag == "Excerpt") {
    if (MScore::noExcerpts)
          e.skipCurrentElement();
    else {
      Excerpt* ex = new Excerpt(this);
      ex->read(e);
      _excerpts.append(ex);
    }
  }
  else if (tag == "Beam") {
    Beam* beam = new Beam(this);
    beam->read(e);
    beam->setParent(0);
    // _beams.append(beam);       // <=
  }
  ....
}

В большом каскаде условий выполняется аллокация памяти. В каждом блоке кода создаётся объект и сохраняется указатель на него. В приведённом фрагменте кода сохранение указателя закомментировали, добавив в код ошибку, приводящую к утечке памяти.

V773 The function was exited without releasing the 'voicePtr' pointer. A memory leak is possible. ove.cpp 3967

bool TrackParse::parse() {
  ....
  Track* oveTrack = new Track();
  ....
  QList<Voice*> voices;
  for( i=0; i<8; ++i ) {
    Voice* voicePtr = new Voice();

    if( !jump(5) ) { return false; }                    // <=

    // channel
    if( !readBuffer(placeHolder, 1) ) { return false; } // <=
    voicePtr->setChannel(placeHolder.toUnsignedInt());

    // volume
    if( !readBuffer(placeHolder, 1) ) { return false; } // <=
    voicePtr->setVolume(placeHolder.toInt());

    // pitch shift
    if( !readBuffer(placeHolder, 1) ) { return false; } // <=
    voicePtr->setPitchShift(placeHolder.toInt());

    // pan
    if( !readBuffer(placeHolder, 1) ) { return false; } // <=
    voicePtr->setPan(placeHolder.toInt());

    if( !jump(6) ) { return false; }                    // <=

    // patch
    if( !readBuffer(placeHolder, 1) ) { return false; } // <=
    voicePtr->setPatch(placeHolder.toInt());

    voices.push_back(voicePtr);                       //SAVE 1
  }

  // stem type
  for( i=0; i<8; ++i ) {
    if( !readBuffer(placeHolder, 1) ) { return false; } // <=
    voices[i]->setStemType(placeHolder.toUnsignedInt());

    oveTrack->addVoice(voices[i]);                    //SAVE 2
  }
  ....
}

Достаточно большой фрагмент, но в наличии ошибки легко убедиться. Каждый отмеченный оператор return приводит к потере указателя voicePtr. Если всё же программа выполняется до строки кода с комментарием «SAVE 2», то указатель сохраняется в класс Track. В деструкторе этого класса и будет выполнено освобождение указателей. В остальных случаях будет утечка памяти. Вот как выглядит реализация класса Track:

class Track{
  ....
  QList<Voice*> voices_;
  ....
}

void Track::addVoice(Voice* voice) {
  voices_.push_back(voice);
}

Track::~Track() {
  clear();
}

void Track::clear(void) {
  ....
  for(int i=0; i<voices_.size(); ++i){
    delete voices_[i];
  }
  voices_.clear();
}

Другие аналогичные предупреждения анализатора лучше посмотреть разработчикам проекта.

Ошибки инициализации



V614 Uninitialized variable 'pageWidth' used. Consider checking the third actual argument of the 'doCredits' function. importmxmlpass1.cpp 944

void MusicXMLParserPass1::scorePartwise()
{
  ....
  int pageWidth;
  int pageHeight;

  while (_e.readNextStartElement()) {
    if (_e.name() == "part")
      part();
    else if (_e.name() == "part-list") {
      doCredits(_score, credits, pageWidth, pageHeight);// <= USE
      partList(partGroupList);
    }
    ....
    else if (_e.name() == "defaults")
      defaults(pageWidth, pageHeight);                 // <= INIT
    ....
  }
  ....
}

Такой код допускает использование неинициализированных переменных pageWidth и pageHeight в функции doCredits():

static
void doCredits(Score* score,const CreditWordsList& credits,
               const int pageWidth, const int pageHeight)
{
  ....
  const int pw1 = pageWidth / 3;
  const int pw2 = pageWidth * 2 / 3;
  const int ph2 = pageHeight / 2;
  ....
}

Использование неинициализированных переменных приводит к неопределённому поведению, что долгое время может создавать видимость правильной работы программы.

V730 Not all members of a class are initialized inside the constructor. Consider inspecting: _dclickValue1, _dclickValue2. aslider.cpp 30

AbstractSlider::AbstractSlider(QWidget* parent)
   : QWidget(parent), _scaleColor(Qt::darkGray),
     _scaleValueColor(QColor("#2456aa"))
{
  _id         = 0;
  _value      = 0.5;
  _minValue   = 0.0;
  _maxValue   = 1.0;
  _lineStep   = 0.1;
  _pageStep   = 0.2;
  _center     = false;
  _invert     = false;
  _scaleWidth = 4;
  _log        = false;
  _useActualValue = false;
  setFocusPolicy(Qt::StrongFocus);
}

double lineStep() const    { return _lineStep; }
void setLineStep(double v) { _lineStep = v;    }
double pageStep() const    { return _pageStep; }
void setPageStep(double f) { _pageStep = f;    }
double dclickValue1() const      { return _dclickValue1; }
double dclickValue2() const      { return _dclickValue2; }
void setDclickValue1(double val) { _dclickValue1 = val;  }
void setDclickValue2(double val) { _dclickValue2 = val;  }
....

К неопределённому поведению может приводить использование неинициализированного поля класса. В этом классе большинство полей инициализируются в конструкторе и имеют методы для доступа к ним. А вот переменные _dclickValue1 и_dclickValue2 остаются не инициализированными, хотя имеют методы для чтения и записи. Если первым будет вызван метод для чтения, то он вернёт неопределённое значение. В коде проекта найдено около ста таких мест, и они заслуживают изучения разработчиками.

Ошибки наследования




V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'adjustCanvasPosition' in derived class 'PianorollEditor' and base class 'MuseScoreView'. pianoroll.h 92

class MuseScoreView {
  ....
  virtual void adjustCanvasPosition(const Element*,
    bool /*playBack*/, int /*staffIdx*/ = 0) {};
  ....
}

class PianorollEditor : public QMainWindow, public MuseScoreView{
  ....
  virtual void adjustCanvasPosition(const Element*, bool);
  ....
}

class ScoreView : public QWidget, public MuseScoreView {
  ....
  virtual void adjustCanvasPosition(const Element* el,
    bool playBack, int staff = -1) override;
  ....
}

class ExampleView : public QFrame, public MuseScoreView {
  ....
  virtual void adjustCanvasPosition(const Element* el,
    bool playBack);
  ....
}

Анализатор нашёл целых три разных способа переопределить и перегрузить функцию adjustCanvasPosition() у базового класса MuseScoreView. Необходимо перепроверить код.

Недостижимый код




V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740

static void readNote(Note* note, XmlReader& e)
{
  ....
  while (e.readNextStartElement()) {
    const QStringRef& tag(e.name());
    if (tag == "Accidental") {
      ....
    }
    ....
    else if (tag == "offTimeType") {        // <= line 651
      if (e.readElementText() == "offset")
        note->setOffTimeType(2);
      else
        note->setOffTimeType(1);
    }
    ....
    else if (tag == "offTimeType")          // <= line 728
      e.skipCurrentElement();               // <= Dead code
    ....
  }
  ....
}

В очень большом каскаде условий присутствуют две одинаковые проверки. При такой ошибке либо не выполняются оба условия, либо выполнится только первое. Таким образом, второе условие никогда не выполняется и код остаётся недостижим.

Ещё два похожих места в коде:

  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 645, 726. read114.cpp 645
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740

Рассмотрим следующую ошибку.

V547 Expression 'middleMeasure != 0' is always false. ove.cpp 7852

bool
getMiddleUnit(...., Measure* middleMeasure, int& middleUnit) {
  ....
}

void OveOrganizer::organizeWedge(....) {
  ....
  Measure* middleMeasure = NULL;
  int middleUnit = 0;

  getMiddleUnit(
    ove_, part, track,
    measure, ove_->getMeasure(bar2Index),
    wedge->start()->getOffset(), wedge->stop()->getOffset(),
    middleMeasure, middleUnit);

  if( middleMeasure != 0 ) {                            // <=
    WedgeEndPoint* midStopPoint = new WedgeEndPoint();
    measureData->addMusicData(midStopPoint);

    midStopPoint->setTick(wedge->getTick());
    midStopPoint->start()->setOffset(middleUnit);
    midStopPoint->setWedgeStart(false);
    midStopPoint->setWedgeType(WedgeType::Cres_Line);
    midStopPoint->setHeight(wedge->getHeight());

    WedgeEndPoint* midStartPoint = new WedgeEndPoint();
    measureData->addMusicData(midStartPoint);

    midStartPoint->setTick(wedge->getTick());
    midStartPoint->start()->setOffset(middleUnit);
    midStartPoint->setWedgeStart(true);
    midStartPoint->setWedgeType(WedgeType::Decresc_Line);
    midStartPoint->setHeight(wedge->getHeight());
    }
  }
  ....
}

Ещё очень большой фрагмент кода, который никогда не выполняется. Причиной является условие, которое всегда ложно. В условии сравнивается с нулём указатель, который изначально инициализирован нулём. При внимательном рассмотрении можно увидеть опечатку: перепутаны переменные middleMeasure и middleUnit. Обратите внимание на функцию getMiddleUnit(). По названию и последнему аргументу (передан по ссылке) видно, что модифицируется переменная middleUnit, которую и надо было проверять в условии.

V547 Expression 'error == 2' is always false. mididriver.cpp 126

#define ENOENT 2

bool AlsaMidiDriver::init()
{
  int error = snd_seq_open(&alsaSeq, "hw", ....);
  if (error < 0) {
    if (error == ENOENT)
      qDebug("open ALSA sequencer failed: %s",
        snd_strerror(error));
    return false;
  }
  ....
}

Очевидно, что после первой проверки переменная error всегда будет меньше нуля. Из-за дальнейшего сравнения переменной с числом 2 никогда не выводится отладочная информация.

V560 A part of conditional expression is always false: strack > — 1. edit.cpp 3669

void Score::undoAddElement(Element* element)
{
  QList<Staff* > staffList;
  Staff* ostaff = element->staff();
  int strack = -1;
  if (ostaff) {
    if (ostaff->score()->excerpt() && strack > -1)
     strack = ostaff->score()->excerpt()->tracks().key(...);
    else
     strack = ostaff->idx() * VOICES + element->track() % VOICES;
  }
  ....
}

Ещё один случай с ошибкой в условном выражении. Всегда выполняется код из else.

V779 Unreachable code detected. It is possible that an error is present. figuredbass.cpp 1377

bool FiguredBass::setProperty(P_ID propertyId, const QVariant& v)
{
  score()->addRefresh(canvasBoundingRect());
  switch(propertyId) {
    default:
      return Text::setProperty(propertyId, v);
    }
  score()->setLayoutAll();
  return true;
}

Диагностика V779 специализирована на поиске недостижимого кода, с ее помощью нашлось вот такое забавное место. И оно не одно в коде, есть ещё два:

  • V779 Unreachable code detected. It is possible that an error is present. fingering.cpp 165
  • V779 Unreachable code detected. It is possible that an error is present. chordrest.cpp 1127

Невалидные указатели/итераторы





V522 Dereferencing of the null pointer 'customDrumset' might take place. instrument.cpp 328

bool Instrument::readProperties(XmlReader& e, Part* part,
  bool* customDrumset)
{
  ....
  else if (tag == "Drum") {
    // if we see on of this tags, a custom drumset will
    // be created
    if (!_drumset)
      _drumset = new Drumset(*smDrumset);
    if (!customDrumset) {                        // <=
      const_cast<Drumset*>(_drumset)->clear();
      *customDrumset = true;                     // <=
    }
    const_cast<Drumset*>(_drumset)->load(e);
  }
  ....
}

Тут допущена ошибка в условии. Скорее всего, автор хотел иначе проверить указатель customDrumset перед разыменованием, но сделал опечатку.

V522 Dereferencing of the null pointer 'segment' might take place. measure.cpp 2220

void Measure::read(XmlReader& e, int staffIdx)
{
  Segment* segment = 0;
  ....
  while (e.readNextStartElement()) {
    const QStringRef& tag(e.name());

    if (tag == "move")
      e.initTick(e.readFraction().ticks() + tick());
    ....
    else if (tag == "sysInitBarLineType") {
      const QString& val(e.readElementText());
      BarLine* barLine = new BarLine(score());
      barLine->setTrack(e.track());
      barLine->setBarLineType(val);
      segment = getSegmentR(SegmentType::BeginBarLine, 0); //!!!
      segment->add(barLine);                           // <= OK
    }
    ....
    else if (tag == "Segment")
      segment->read(e);                                // <= ERR
    ....
  }
  ....
}

Это уже не первый большой каскад условий в этом проекте, где допускают ошибки. Стоит задуматься! Здесь указатель segment изначально равен нулю, а перед использованием инициализируется при разных условиях. В одной ветви это сделать забыли.

Ещё два опасных места:

  • V522 Dereferencing of the null pointer 'segment' might take place. read114.cpp 1551
  • V522 Dereferencing of the null pointer 'segment' might take place. read206.cpp 1879

V774 The 'slur' pointer was used after the memory was released. importgtp-gp6.cpp 2072

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;
  }
}

Указатель slur используется уже после освобождения памяти с помощью оператора delete. Вероятно, тут перепутали строки местами.

V789 Iterators for the 'oldList' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. layout.cpp 1760

void Score::createMMRest(....)
{
  ElementList oldList = mmr->takeElements();

  for (Element* ee : oldList) {    // <=
    if (ee->type() == e->type()) {
      mmr->add(ee);
      auto i = std::find(oldList.begin(), oldList.end(), ee);
      if (i != oldList.end())
        oldList.erase(i);          // <=
      found = true;
      break;
    }
  }
  ....
}

Анализатор обнаружил одновременное чтение и модификацию контейнера oldList в range-based for цикле. Такой код является ошибочным.

Ошибки с арифметикой



V765 A compound assignment expression 'x += x + ...' is suspicious. Consider inspecting it for a possible error. tremolo.cpp 321

void Tremolo::layout()
{
  ....
  if (_chord1->up() != _chord2->up()) {
    beamYOffset += beamYOffset + beamHalfLineWidth; // <=
  }
  else if (!_chord1->up() && !_chord2->up()) {
    beamYOffset = -beamYOffset;
  }
  ....
}

Вот такой код нашёл анализатор. Указанное выражение равносильно такому:

beamYOffset = beamYOffset + beamYOffset + beamHalfLineWidth;

Переменная beamYOffset складывается два раза. Возможно, это является ошибкой.

V674 The '-2.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'alter < — 2.5' expression. importmxmlpass2.cpp 5253

void MusicXMLParserPass2::pitch(int& step, int& alter ....)
{
  ....
  alter = MxmlSupport::stringToInt(strAlter, &ok);
  if (!ok || alter < -2.5 || alter > 2.5) {
    logError(QString("invalid alter '%1'").arg(strAlter));
    ....
    alter = 0;
  }
  ....
}

Переменная alter имеет целочисленный тип int. И сравнение с числами 2.5 и -2.5 выглядит очень странным.

V595 The 'sample' pointer was utilized before it was verified against nullptr. Check lines: 926, 929. voice.cpp 926

void Voice::update_param(int _gen)
{
 ....
 if (gen[GEN_OVERRIDEROOTKEY].val > -1) {
  root_pitch = gen[GEN_OVERRIDEROOTKEY].val * 100.0f - ....
 }
 else {
  root_pitch = sample->origpitch * 100.0f - sample->pitchadj;
 }
 root_pitch = _fluid->ct2hz(root_pitch);
 if (sample != 0)
  root_pitch *= (float) _fluid->sample_rate / sample->samplerate;
 break;
  ....
}

Анализатор ругается на разыменование непроверенного указателя sample, когда ниже по коду проверка присутствует. Но что если указатель sample не планировали проверять в этой функции, а с нулём хотели сравнить переменную sample->samplerate перед делением? Тогда здесь имеет место серьёзная ошибка.

Разное




V523 The 'then' statement is equivalent to the 'else' statement. pluginCreator.cpp 84

PluginCreator::PluginCreator(QWidget* parent)
   : QMainWindow(parent)
{
  ....
  if (qApp->layoutDirection() == Qt::LayoutDirection::....) {
    editTools->addAction(actionUndo);
    editTools->addAction(actionRedo);
  }
  else {
    editTools->addAction(actionUndo);
    editTools->addAction(actionRedo);
  }
  ....
}

Анализатор обнаружил выполнение одинакового кода при разных условиях. Тут следует исправить ошибку, либо сократить код вдвое, убрав условие.

V524 It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function. rest.cpp 667

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() имеют противоположные по смыслу названия, но при этом реализованы одинаково. Стоит проверить это подозрительное место.

V766 An item with the same key '«mrcs»' has already been added. importgtp-gp6.cpp 100

const static std::map<QString, QString> instrumentMapping = {
  ....
  {"e-piano-gs", "electric-piano"},
  {"e-piano-ss", "electric-piano"},
  {"hrpch-gs", "harpsichord"},
  {"hrpch-ss", "harpsichord"},
  {"mrcs", "maracas"},                // <=
  {"mrcs", "oboe"},                   // <=
  {"mrcs", "oboe"},                   // <= явный Copy-Paste
  ....
};

Похоже автор этого фрагмента кода торопился и насоздавал пар с одинаковыми ключами, но разными значениями.

V1001 The 'ontime' variable is assigned but is not used until the end of the function. rendermidi.cpp 1176

bool renderNoteArticulation(....)
{
  int ontime    = 0;
  ....
  // render the suffix
  for (int j = 0; j < s; j++)
    ontime = makeEvent(suffix[j], ontime, tieForward(j,suffix));
  // render graceNotesAfter
  ontime = graceExtend(note->pitch(), ...., ontime);
  return true;
}

В коде модифицируется переменная ontime, но при этом не используется при выходе из функции. Возможно, тут допущена ошибка.

V547 Expression 'runState == 0' is always false. pulseaudio.cpp 206

class PulseAudio : public Driver {
  Transport state;
  int runState;           // <=
  ....
}

bool PulseAudio::stop()
{
  if (runState == 2) {
    runState = 1;
    int i = 0;
    for (;i < 4; ++i) {
      if (runState == 0)  // <=
        break;
      sleep(1);
    }
    pthread_cancel(thread);
    pthread_join(thread, 0);
    }
  return true;
}

Анализатор обнаружил всегда ложное условие, но функция stop() вызывается в параллельном коде и срабатывания быть не должно. Причина появления предупреждения заключается в том, что автор кода воспользовался для синхронизации простой переменой типа int, являющейся полем класса. А это приводит к ошибкам синхронизации. После исправления кода, диагностика V547 перестанет выдавать ложное срабатывание, т.е. в ней сработает исключение на тему параллельного кода.

Заключение


В небольшом проекте нашлось много разных ошибок. Надеемся, авторы программы обратят внимание на мой обзор и проведут исправительные работы. Я проверю код ещё нескольких программ, которыми пользуюсь. Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту.

Попробовать анализатор PVS-Studio на своём проекте очень легко, достаточно перейти на страницу загрузки.



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Review of Music Software Code Defects Part 1. MuseScore

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.

Комментарии (24)


  1. JoeyBlack
    27.09.2017 11:43

    серьёзный анализ


  1. prostofilya
    27.09.2017 12:11
    +2

    Эх, вот бы глянуть что внутри у монстров вроде cubase, ableton, fl studio, reason…


    1. mars0h0d
      27.09.2017 20:37

      Поддерживаю :)
      Особенно, насчет Cubase


  1. vesper-bot
    27.09.2017 12:34
    +1

    Похоже, такой ассортимент багов дал пищу не только авторам статьи, но и дизайнеру единорожек...


  1. neumeika
    27.09.2017 12:46

    ТС, Вам и «десигнеру картинок» в очередной раз спасибо =)
    В музыкалке у нас такого не преподавали %)


    1. EvgeniyRyzhkov
      27.09.2017 12:50

      ТС сам «микшировал» картинки :-)


  1. comargo
    27.09.2017 13:50
    +1

    Возник вопрос по поводу первой утечки памяти…
    Вполне вероятно (код не читал) классы Beam и MasterScore являются наследниками QObject (тут же у нас Qt код), а значит при создании экземпляра класса Beam в список "детей" MasterScore'а записывается этот экземпляр, который будет удалён по удалению "отца", а значит утечки не будет.
    Хотя в любом случае код является "подозрительным".


    1. mayorovp
      27.09.2017 14:03

      Нет, этого не происходит:


          Beam::Beam(Score* s) : Element(s)
          Element::Element(Score* s) : QObject(0), ScoreElement(s)
          ScoreElement(Score* s) : _score(s)   {}


  1. Daddy_Cool
    27.09.2017 13:50
    +3

    К сожалению, ни одного удобного редактора для работы с нотами на сегодня нет, и MusicScore не исключение. Почему-то разработчики ориентируются на последующую печать, что вообще неактуально. А в Кубэйсе в нотами вообще full-атас.


    1. SvyatoslavMC Автор
      27.09.2017 14:08
      +1

      Finale NotePad довольно удобный редактор нот. Но он платный, есть только месячный триал.


    1. urtow
      27.09.2017 14:52
      +1

      MuseScore при всех своих недостатках имеет большой плюс — он работает и работает почти везде:

      — Win/Linux/Mac
      — iOS/Andriod планшеты

      Найти ему альтернативу не получилось ну никак, хоть и приходится материться на него и ждать фиксов багов.


      1. shoorick
        28.09.2017 11:07

        Andriod планшеты

        Да ну? То, что под названием MuseScore присутствует в Google Play Market, умеет только играть ранее загруженные ноты.


        1. urtow
          28.09.2017 13:03
          -1

          Под iOS так же.

          Но по факту для пданшета больше и не надо или вы хотели полный набор функций на планшете?


    1. nikitazu
      27.09.2017 16:42
      +2

      Lilypond весьма удобен, набираешь в текстовом редакторе на специальном языке разметки, потом компилируешь в PDF/midi.


      1. shoorick
        28.09.2017 10:21
        +1

        Лилипонд хорош — не вопрос. Только у меня в нём не получается быстро набирать: набор листа четырёхголосной хоровой партитуры занимает 75 минут, а в случае, когда надо ещё и партию фортепиано добавить, всё замедляется ещё в несколько раз. Поэтому для сложных нот всё-таки приходится брать MuseScore и либо полностью всё в нём делать, либо полностью набранное перегонять через MusicXML в Лилипонд, чтоб результат лучше выглядел.


        1. SvyatoslavMC Автор
          28.09.2017 10:43

          Я пока собирал софт для анализа, встретил Denemo — графический интерфейс для LilyPond. Возможно, вам пригодится.


          1. shoorick
            28.09.2017 11:13

            Денемо тоже пробовал — уже перестал. Да и графическим интерфейсом к Лилпонду я бы считал, скорее, Frescobaldi — комбайн из текстового редактора, просмотрщика логов и PDF с возможностью перейти по клику на ноте в конкретный участок исходного кода.


        1. Kobalt_x
          29.09.2017 23:39

          я же правильно понимаю, что вы имеете в виду набор именно руками с клавы, не с помощью midi-in?


    1. mike1
      27.09.2017 18:47

      А Sibelius?


      1. shoorick
        28.09.2017 10:15

        Сибелиус — платный и под винду/макось. А Музскор — под всё и на халяву. К тому же, в Музскоре дофига общего с третьим Сибелиусом вплоть до раскраски голосов и иногда совпадающих клавиш.


  1. Alexeyslav
    27.09.2017 13:53

    Удивительно, как с такими ошибками программа выполняет свои функции…


  1. Daddy_Cool
    27.09.2017 21:11
    +3

    Final и Sibelius тоже пробовали. ) Lilypond — надо, как никак ТеХ я юзаю регулярно.
    Моя задача, не в том, чтобы получить нотный лист, а в том чтобы музыка была перед глазами и было удобно её сочинять. Т.е. нужен редактор/секвенсор.
    Самый удобный редактор/секвенсор на мой (и не только — в одной обзорной статье его назвали «идеальным») взгляд — это Midisoft Studio 4. Увы, это продукт 90-х годов, чтобы его запустить сейчас нужна виртуалка. Там многого не хватает, но то что есть — работает очень удобно. Нашлась фирма которая попыталась подхватить упавшее знамя и выпустила редактор Forte Notation, как они говорили — на базе Studio — но увы — существенные элементы интерфейса были не воспроизведены, а добавленные — не актуальны.
    В результате я пользуюсь зверинцем из Studio под виртуальной XP, иногда Сибелиусом, ну и окончательно все доделывается в Cubase.


    1. shoorick
      28.09.2017 10:26

      Я на днях у себя наткнулся на ноты, которые набирал в Midisoft Studio пятнадцать лет назад — теперь думаю, что же проще: то ли старый компьютер с Windows XP раскочегарить и туда Midisoft Studio поставить, то ли набрать снова — в MuseScore или Лилипонде.


      1. Daddy_Cool
        28.09.2017 11:16

        Под виртуалкой Studio работает терпимо — иногда нарушается темп, а в общем всё хорошо.
        По моему сейчас произошел откат от нотной системы в сторону всяких визуальных штук (те же гитарные табы или полосы длительности в Cubase) — в связи с этим нотные миди-редакторы не развиваются.