👩🏾‍✈️ 😪 👄 OpenToonz: Di Dalam dan Di Luar 👰🏾 👧🏽 👩🏾‍🎨

Hampir empat tahun telah berlalu sejak PVS-Studio memverifikasi kode sumber OpenToonz. Proyek ini adalah alat yang sangat kuat untuk membuat animasi dua dimensi. Sejak cek terakhir, dengan bantuannya, karya animasi seperti Mary dan Bunga Penyihir, Batman Ninja, Promare, dan lainnya telah dibuat. Karena studio besar terus menggunakan Toonz, mengapa tidak memeriksa kualitas kode sumber lagi?

Anda bisa berkenalan dengan analisis kesalahan sebelumnya dalam artikel " Kode buruk paket untuk membuat animasi 2D Toonz ". Secara umum, tampaknya kualitas kode tidak meningkat banyak, sehingga kesan keseluruhannya tidak lebih baik. Selain itu, banyak kesalahan yang sama ditemukan di artikel sebelumnya. Kami tidak akan mempertimbangkannya lagi, karena ada banyak hal untuk dipilih.

Namun, Anda harus memahami bahwa keberadaan kesalahan tidak serta merta mencegah penggunaan aktif dan produktif dari produk perangkat lunak. Kemungkinan besar, kesalahan ditemukan langsung di bagian kode yang jarang digunakan atau sama sekali tidak terpakai, jika tidak mereka akan diidentifikasi dalam proses menggunakan aplikasi dan diperbaiki. Namun, ini tidak berarti bahwa analisis statis adalah berlebihan. Hanya saja, makna analisis statis bukanlah untuk mencari kesalahan lama dan tidak relevan, tetapi untuk mengurangi biaya proses pengembangan. Banyak kesalahan saat menulis kode dapat dideteksi dengan segera, dan tidak selama pengoperasian perangkat lunak. Oleh karena itu, dengan penggunaan penganalisis statis secara teratur, kesalahan diperbaiki pada tahap awal. Ini menghemat waktu pengembang dan uang perusahaan, dan meningkatkan persepsi produk oleh pengguna. Setuju, itu tidak menyenangkan untuk terus berhenti berlangganan ke pengembang,bahwa satu hal atau yang lain tidak bekerja.

Fragmen N1

V610 Perilaku tidak terdefinisi. Periksa operator shift '<<'. Operan kiri '(- 1)' negatif.

decode_mcu_AC_refine (j_decompress_ptr cinfo, JBLOCKROW *MCU_data)
{
  int p1, m1;
  p1 = 1 << cinfo->Al;    
  m1 = (-1) << cinfo->Al; 
  ....
}

Tidak jelas apa yang ingin dicapai oleh pembuat kode di sini. Menggunakan operator shift dengan angka negatif menyebabkan perilaku yang tidak terdefinisi. Cara perilaku operator shift dijelaskan dalam standar tampaknya sedikit membingungkan pada pandangan pertama, tetapi bagaimanapun, mari kita periksa:

1. Jenis hasilnya adalah operan kiri yang dipromosikan. Perilaku tidak terdefinisi jika operan kanan negatif, atau lebih besar dari atau sama dengan panjang dalam bit dari operan kiri yang dipromosikan.

2. Nilai E1 << E2 adalah posisi bit E2 bergeser kiri E1; bit yang dikosongkan diisi nol. Jika E1 memiliki jenis yang tidak ditandatangani, nilai hasilnya adalah E1 * 2 ^ E2, mengurangi modulo satu lebih dari nilai maksimum yang diwakili dalam jenis hasil. Jika tidak, jika E1 memiliki tipe bertanda tangan dan nilai non-negatif, dan E1 * 2 ^ E2 diwakili dalam tipe hasil, maka itu adalah nilai yang dihasilkan; jika tidak, perilaku tidak terdefinisi.

Jadi, perilaku tidak didefinisikan jika operan kanan atau kiri memiliki nilai negatif. Jika operan adalah tipe bertanda, nilai non-negatif dan cocok dengan tipe yang dihasilkan, maka perilaku akan menjadi normal.

Fragmen N2

V517Penggunaan pola 'if (A) {...} else if (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 156, 160. cameracapturelevelcontrol.cpp 156

void CameraCaptureLevelHistogram::mousePressEvent(QMouseEvent* event) {
  if (event->button() != Qt::LeftButton) return;
  if (m_currentItem == Histogram) {
    m_histogramCue = true;
    return;
  }
  if (m_currentItem == None) return;
  QPoint pos = event->pos();
  if (m_currentItem == BlackSlider)  // <=
    m_offset = pos.x() - SIDE_MARGIN - m_black;
  else if (m_currentItem == GammaSlider)
    m_offset = pos.x() - SIDE_MARGIN - gammaToHPos(m_gamma, m_black, m_white);
  else if (m_currentItem == BlackSlider)  // <=
    m_offset = pos.x() - SIDE_MARGIN - m_white;
  else if (m_currentItem == ThresholdSlider)
    m_offset = pos.x() - SIDE_MARGIN - m_threshold;
}

Di sini variabel m_offset diberi nilai yang berbeda tergantung pada nilai m_currentItem . Namun, menduplikasi cek pada BlackSlider tidak ada gunanya, dan dari tubuh kondisi Anda dapat melihat bahwa variabel m_white terlibat dalam perhitungan . Mari kita lihat nilai yang mungkin untuk m_currentItem .

  LevelControlItem m_currentItem;

  enum LevelControlItem {
    None = 0,
    BlackSlider,
    WhiteSlider,
    GammaSlider,
    ThresholdSlider,
    Histogram,
    NumItems
  };

Ternyata nilai WhiteSlider juga dimungkinkan , yang verifikasi tidak dilakukan. Dengan demikian, sangat mungkin bahwa beberapa skenario perilaku hilang karena kesalahan salin-tempel.

Fragmen N3

V517 Penggunaan pola 'jika (A) {...} else jika (A) {...}' terdeteksi. Ada kemungkinan kehadiran kesalahan logis. Periksa baris: 784, 867. tpalette.cpp 784

void TPalette::loadData(TIStream &is) {
  ....
  std::string tagName;
  while (is.openChild(tagName)) {
    if (tagName == "version") {
      ....
    } else if (tagName == "stylepages") { // <=
      while (!is.eos()) {
        if (....){
          ....
        }
        ....
        is.closeChild();
        }
    } else if (tagName == "refImgPath") {
      ....
    } else if (tagName == "animation") {
      ....
    } else if (tagName == "stylepages") { // <=
      int key = '0';
      while (!is.eos()) {
        int styleId = 0;
        ....
      }
    } 
      ....
  }
}

Kesalahan serupa lainnya. Di sini, kondisi yang sama memiliki badan yang berbeda, tetapi tidak lagi mungkin untuk menyimpulkan tentang opsi yang mungkin untuk nilai tagName . Kemungkinan besar, hanya beberapa opsi yang terlewatkan, dan pada akhirnya kami memiliki kode yang tidak akan pernah dieksekusi.

Fragmen N4

V547 Ekspresi 'chancount == 2' selalu benar. psd.cpp 720

void TPSDReader::readImageData(...., int chancount) {
  ....
  if (depth == 1 && chancount == 1) { // <= 1
    ....
  } else if (depth == 8 && chancount > 1) {
    ....
    for (....) {
      if (chancount >= 3) {
        ....
        if (chancount == 4)  
          ....
        else
          ....
      } else if (chancount <= 2)  // <= 2
      {
        ....
        if (chancount == 2) // <= 3
          ....
        else
          ....
      }
      ....
    }
    ....
  } else if (m_headerInfo.depth == 8 && chancount == 1) {
  ....
}

Kesalahan logis kecil merayapi pemeriksaan ini. Dalam tes di bawah nomor satu, chancount dibandingkan dengan 1, dan dalam uji nomor 2, diperiksa bahwa variabel ini kurang dari atau sama dengan 2. Sebagai hasilnya, satu-satunya nilai chancount yang mungkin adalah 2 dengan kondisi di bawah angka 3. Kesalahan ini tidak dapat menyebabkan operasi program yang salah, tapi itu menyulitkan pembacaan dan pemahaman kode. Sebagai contoh, tidak jelas mengapa cabang-lain kalau begitu ...

Secara umum, fungsi yang dipertimbangkan dalam fragmen ini membutuhkan sedikit lebih dari 300 baris kode dan terdiri dari tumpukan kondisi dan loop.

Fragment N5

V614 Variabel tidak diinisialisasi 'precSegmentIndex' digunakan. Pertimbangkan untuk memeriksa argumen aktual kelima dari fungsi 'insertBoxCorners'. rasterelection.cpp 803

TStroke getIntersectedStroke(TStroke &stroke, TRectD bbox) {
  ....
  int precSegmentIndex, currentSegmentIndex, startSegmentIndex,
      precChunkIndex = -1;
  ....
  if (....) {
    insertBoxCorners(bbox, points, outPoints, currentSegmentIndex,
                     precSegmentIndex);
    ....
  }
}

void insertBoxCorners(...., int currentSegmentIndex, int precSegmentIndex) {
  ....
  bool sameIndex = (precSegmentIndex == currentSegmentIndex);
  ....
  int segmentIndex = precSegmentIndex;
  ....
}

Mungkin, kesalahan dibuat di sini bahkan selama inisialisasi variabel precSegmentIndex , currentSegmentIndex , startSegmentIndex , precChunkIndex . Pengembang dapat berharap bahwa inisialisasi elemen terakhir -1 menginisialisasi dengan nilai yang sama dengan variabel lain yang dideklarasikan pada baris yang sama.

Fragmen N6

V590 Pertimbangkan memeriksa ekspresi 's! = "" && s == "warna". Ekspresi berlebihan atau mengandung kesalahan cetak. cleanupparameters.cpp 416

void CleanupParameters::loadData(TIStream &is, bool globalParams) {
  ....
  std::string s = is.getTagAttribute("sharpness");
  ....
  if (....)
  {
    ....
  } else if (tagName == "lineProcessing")
    ....
    if (s != "" && isDouble(s)) 
      ....
    if (s != "" && isDouble(s))
      ....
    if (s != "" && s == "color") // <=
      ....
  } else if (tagName == "despeckling") {
    ....  
  }
  ....
}

Kesalahan ini, bahkan agak cacat, dengan sendirinya hanya mengarah pada perbandingan yang tidak perlu. Namun, jika kita melihat kode secara keseluruhan, akan menjadi jelas bahwa perbandingan tambahan muncul sebagai hasil dari copy-paste dari kondisi sebelumnya.

Mie seperti itu, yang menempati lusinan atau lebih baris kode, mungkin mengandung kesalahan logika lainnya, dan pencarian mereka dengan format ini dapat berubah menjadi siksaan.

Fragment N7

V772 Memanggil operator 'delete' untuk void pointer akan menyebabkan perilaku yang tidak terdefinisi. pluginhost.cpp 1327

static void release_interface(void *interf) {
  if (interf) delete interf;
}

Di sini pesan analyzer itu sendiri sudah cukup lengkap: memanggil operator hapus pada pointer ke tipe void mengarah ke perilaku yang tidak terdefinisi. Jika fungsi universal diperlukan untuk menghapus antarmuka, mungkin ada baiknya menjadikannya boilerplate.

template<class T>
static void release_interface(T *interf) {
  if (interf) delete interf;
}

Fragment N8

V568 Aneh bahwa operator 'sizeof ()' mengevaluasi ukuran pointer ke kelas, tetapi bukan ukuran objek kelas 'm_xshHandle'. tstageobjectcmd.cpp 455

class DVAPI TStageObjectParams {
  ....
};

class RemovePegbarNodeUndo final : public TUndo {
  ....
  TXsheetHandle *m_xshHandle;

public:
  int getSize() const override {
    return sizeof *this + sizeof(TStageObjectParams) + sizeof(m_xshHandle);
  }
  ....
}

Kesalahan yang cukup umum yang bisa terjadi baik melalui kurangnya perhatian dan ketidaktahuan. Di sini, kemungkinan besar, masalah itu tidak diperhatikan, karena pada periode pertama ini tetap saja dereferensi. Jika Anda membutuhkan ukuran objek, Anda harus selalu ingat bahwa penunjuk objek harus ditinjau ulang. Kalau tidak, kita hanya mendapatkan ukuran dari pointer itu sendiri.

return sizeof *this + sizeof(TStageObjectParams) + sizeof(*m_xshHandle);

Fragmen N9

V568 Aneh bahwa operator 'sizeof ()' mengevaluasi ukuran pointer ke kelas, tetapi bukan ukuran objek kelas 'ini'. shaderfx.cpp 107

struct RectF {
  GLfloat m_val[4];
  ....
  bool operator==(const RectF &rect) const {
    return (memcmp(m_val, rect.m_val, sizeof(this)) == 0);
  }
};

Dan di sini, jelas, mereka lupa untuk menunjuk pointer ini . Akibatnya, kita tidak mendapatkan ukuran objek, tetapi ukuran pointer. Akibatnya, hanya 4 atau 8 byte pertama yang dibandingkan (tergantung pada kedalaman bit). Versi kode yang benar:

return (memcmp(m_val, rect.m_val, sizeof(*this)) == 0);

Fragmen N10

V554 Penggunaan unique_ptr salah. Memori yang dialokasikan dengan 'baru [] akan dibersihkan menggunakan' hapus '. screensavermaker.cpp 29

void makeScreenSaver(TFilePath scrFn, TFilePath swfFn,
                     std::string screenSaverName) {
  struct _stat results;
....
  int swfSize = results.st_size;
  std::unique_ptr<char> swf(new char[swfSize]);
....
}

Seringkali dilupakan bahwa tipe instantiated unique_ptr bergantung pada apakah hapus atau hapus [] akan digunakan. Akibatnya, jika Anda instantiate pointer seperti pada fragmen yang dipertimbangkan, saat mengalokasikan memori melalui [] baru, perilaku yang tidak terdefinisi dapat terjadi, karena rilis akan terjadi melalui penghapusan. Untuk menghindari ini, Anda perlu menambahkan tanda kurung siku ke tipe pointer: std :: unique_ptr <char []>.

Fragmen N11

V521 Ekspresi seperti itu menggunakan operator ',' berbahaya. Pastikan ungkapan 'm_to, m_from = it-> first.getNumber ()' benar. flipbook.cpp 509

class LoadImagesPopup final : public FileBrowserPopup {
  ....
  int m_from, m_to, ....;
  ....
}
void LoadImagesPopup::onFilePathClicked(....) {
  TLevel::Iterator it;
  ....
  it = level->begin();
  m_to, m_from = it->first.getNumber();  // <=
  for (; it != level->end(); ++it) m_to = it->first.getNumber();

  if (m_from == -2 && m_to == -2) m_from = m_to = 1;

  m_minFrame = m_from;
  m_maxFrame = m_to;
  ....
}

Mungkin programmer berharap Anda bisa memberikan satu nilai ke beberapa variabel hanya dengan menuliskannya dengan koma. Namun, operator "," di C ++ bekerja secara berbeda. Di dalamnya, operan pertama dieksekusi, dan hasilnya adalah "reset", maka operan kedua dihitung. Dan, meskipun variabel m_to diinisialisasi dalam siklus berikutnya, jika ada yang tidak beres atau seseorang secara tidak akurat refactor, ada kemungkinan bahwa m_to tidak akan menerima nilai. Dan secara umum, dalam hal apapun, kode ini terlihat aneh.

Fragmen N12

V532 Pertimbangkan untuk memeriksa pernyataan pola '* pointer ++'. Mungkin berarti: '(* pointer) ++'. trop.cpp 140

template <class T, class Q>
void doGammaCorrect(TRasterPT<T> raster, double gamma) {
  Gamma_Lut<Q> lut(....);

  int j;
  for (j = 0; j < raster->getLy(); j++) {
    T *pix    = raster->pixels(j);
    T *endPix = pix + raster->getLx();
    while (pix < endPix) {
      pix->r = lut.m_table[pix->r];
      pix->b = lut.m_table[pix->b];
      pix->g = lut.m_table[pix->g];
      *pix++; // <=
    }
  }
}

Kelemahan kecil, yang selanjutnya dapat membingungkan orang yang membaca kode. Peningkatan, sebagaimana dimaksud, menggeser pointer. Namun, setelah ini ada dereferencing yang tidak berarti. Lebih baik menulis pix ++ .

Fragment N13

V773 Fungsi ini keluar tanpa melepaskan pointer 'autoCloseUndo'. Kebocoran memori dimungkinkan. vectortapetool.cpp 575

void joinLineToLine(....) {
  ....
  UndoAutoclose *autoCloseUndo = 0;
  ....
  autoCloseUndo = new UndoAutoclose(....);
  ....
  if (pos < 0) return;
  ....
  TUndoManager::manager()->add(autoCloseUndo);
}

Ada lebih dari 20 peringatan seperti itu. Seringkali di suatu tempat di akhir fungsi memori dibebaskan, tetapi untuk kembali sebelumnya langkah yang diperlukan ini dilewati. Jadi di sini. Pada akhirnya, pointer diteruskan ke TUndoManager :: manager () -> add () , yang akan menghapus penghapusan pointer, tetapi ada pengembalian di atas di mana Anda lupa memanggil metode ini. Jadi selalu diingat pointer Anda setiap kali Anda keluar fungsi, dan bukan hanya masuk ke suatu tempat delete di ujung blok atau sebelum yang terakhir kembali .

Namun, jika untuk versi singkat kode kesalahan ini tampak jelas, maka dalam kode nyata, lebih rumit, melacak masalah seperti itu bisa sulit. Di sini, penganalisa statis yang selalu lelah akan membantu.

Fragmen N14

V522 Dereferencing dari 'pointer' null pointer mungkin terjadi. palettecmd.cpp 94

bool isStyleUsed(const TVectorImageP vi, int styleId) {
  ....
  int regionCount = vi->getRegionCount();
  for (i = 0; i < regionCount; i++) {
    TRegion *region = vi->getRegion(i);
    if (region || region->getStyle() != styleId) return true;
  }
  ....
}

Di sini kita dapat mengasumsikan bahwa pengembang mencampur aturan evaluasi hubung singkat dan berpikir bahwa jika cek pertama dari pointer mengembalikan false , maka dereferencing dari null pointer tidak akan terjadi. Namun, untuk operator "||" justru sebaliknya.

Fragmen N15

V561 Mungkin lebih baik untuk memberikan nilai pada variabel 'ca' daripada mendeklarasikannya lagi. Deklarasi sebelumnya: xshcellmover.cpp, baris 319. xshcellmover.cpp 323

V561 Mungkin lebih baik untuk memberikan nilai pada variabel 'cb' daripada mendeklarasikannya lagi. Deklarasi sebelumnya: xshcellmover.cpp, baris 320. xshcellmover.cpp 324xshcellmover.cpp 323

void redo() const override {
  int ca       = m_cellsMover.getStartPos().x;
  int cb       = m_cellsMover.getPos().x;
  ....
  if (!m_cellsMover.getOrientation()->isVerticalTimeline()) {
    int ca = m_cellsMover.getStartPos().y;
    int cb = m_cellsMover.getPos().y;
  }
  ....
  if (ca != cb) {
    ....
  }
  ....
}

Mungkin salin-rekat lain, tetapi dengan esensi kesalahan yang tidak biasa. Panggilan ke x digantikan oleh y , tetapi mereka lupa untuk menghapus jenis variabel di awal baris, karena yang pelaporan ulang lokal terjadi. Akibatnya, alih-alih mengubah orientasi posisi untuk ca dan cb awal , ca lokal dan cb baru dibuat , yang dengannya tidak ada lagi yang terjadi. Tetapi ca dan cb eksternal terus ada dengan nilai untuk x .

Kesimpulan N1

Dalam proses menulis artikel, menjadi menarik bagi saya untuk menyodok dan mencoba melakukan sesuatu dalam program ini. Mungkin aku beruntung, tapi perilaku aneh tidak lama datang: hang, menampilkan manipulasi saya tablet setelah kendur dan alun-alun aneh dengan menekan Ctrl + Z . Sayangnya, saya tidak dapat mengulangi perilaku ini.

Tetapi pada kenyataannya, terlepas dari perilaku ini dan menanamkan kebiasaan menekan Ctrl + S secara teratur , OpenToonz mengesankan dengan skala dan fungsionalitasnya. Tetap saja, tidak sia-sia bahwa studio besar juga menggunakannya.

Dan seni saya sebagai bonus:

Kesimpulan N2

Dalam kasus OpenToonz, jelas bahwa mencoba untuk memperbaiki semua kesalahan yang terdeteksi oleh alat analisa sekaligus akan menjadi tugas besar yang akan menghambat proses pengembangan. Untuk kasus-kasus seperti itu, ada pendekatan "Penindasan Massal", ketika utang teknis dimasukkan ke dalam basis penekan alat analisis dan pekerjaan lebih lanjut dengan alat analisis dilakukan berdasarkan respons baru. Nah, jika waktu muncul, maka Anda bisa memilah utang teknis.

PS Saya mengingatkan Anda bahwa pengembang proyek sumber terbuka dapat menggunakan opsi lisensi gratis PVS-Studio . Jika Anda ingin berbagi artikel ini dengan audiens yang berbahasa Inggris, silakan gunakan tautan terjemahan: Victoria Khanieva. OpenToonz .

OpenToonz: Di Dalam dan Di Luar

More articles: