Fix issues detected by static analysis
Open, Needs TriagePublic

Description

Why?

Static analysis is how computers help us with code review. It’s a tool that churns through the codebase and flags potential errors and bugs, both security and functional: uninitialized variables, null pointer dereferences, resource leaks, dead code and many more. Fixing those issues not only contributes to the overall code quality and makes development less tedious: it has a direct impact on the artists’ experience while working with Krita.

What needs to be done?

We currently have over 600 issues tracked in the Coverity Scan database. Some of them may be false positives and need to be flagged as such. The others need to be fixed.

Before you get started

  1. We look forward to meeting you :) Visit us on the IRC: https://docs.krita.org/en/contributors_manual/community.html#internet-relay-chat
  2. Setup a development environment and build Krita from source: https://docs.krita.org/en/untranslatable_pages/intro_hacking_krita.html
  3. Add yourself to the Krita project in Coverity: https://scan.coverity.com/projects/krita

Fixing issues

  1. Pick an issue from the database. Investigate.
  2. If it’s a false positive, change “Classification” to “False Positive” and “Action” to “Ignore”. If your access is read-only, we can update the database for you.
  3. If it’s not a false positive, fix the issue:
    1. If you are unsure about the proper way of fixing, you can consult our code style guide (https://invent.kde.org/graphics/krita/-/blob/master/HACKING), the context information in Coverity, or ask on the IRC
    2. Build and test your change and/or run relevant unit tests.
    3. Commit your change. Create one commit for every issue (unless it's impossible and you need to fix multiple at once). At the end of the commit message, append "CID <issue number>"
    4. File a merge request on Invent. If you plan on fixing more than one issue, please, group the fixes into one merge request (~10 issues per MR). Make sure to first inform us in some way you're working on them - you can assign yourself to the issues in the Coverity Scan database if you have access, or just create a list of those CIDs and share it with us, we'll assign you.

Issues for beginners: unitialized variables and how to fix them
One of the easiest Coverity issues are those that refer to a variable that wasn't initialized in the constructor. Krita has plenty of them, they are all very similar, so even a beginner in coding can try to fix them.

  1. Find appropriate issue. Issues of this kind have field Type with either of those two values: Uninitialized scalar field or Uninitialized pointer field.
  2. Check which file the issue is about. If it's a normal class or struct, it will be defined in .h file and has a constructor and other things in .cpp file. It is however possible that it is a private struct or class, which means it's defined in .cpp file and only used inside that file.
  3. When fixing a class, please make sure that the whole file with this class definition is fixed, which means it works according to those standards:
    1. All fields of types: bool, int, float, double, quintXXX etc. and all pointers are initialized in the definition using the following style: int m_variable {0}; (name here is just an example - don't rename any variables, please). Make sure to initialize them to the default value for that class - especially with bools, you might need to check which one is the default one.
    2. Constructors should use initialization lists only to initialize non-default values.
  4. After fixing one file, make extra sure that:
    1. every class in that file has been fixed,
    2. the new initialization system doesn't change the behaviour of the class (meaning: it will always be initialized either just how it was initialized, or how it is supposed to be initialized),
    3. you haven't removed any important initializations to non-default values,
    4. there is no initializations to default values left in constructors,
    5. you initialized all fields that Coverity issue complained about.
  5. If you're not sure about whether a specific type of field needs to be initialized, please just ask. Note: SP pointers, QVector<> etc. fields don't need initialization.
  6. Bonus tip: if you manage to find tiar on IRC, she can give you a script that might or might not help to initialize the fields in some easiest cases. However lots of those issues needs manual work, and even those that do work with the script needs careful supervision, so you might prefer to do it all manually.
tymond updated the task description. (Show Details)Aug 1 2020, 10:06 PM
amedonosova renamed this task from Fix issues detected by static analysis [PROPOSAL] to Fix issues detected by static analysis.Aug 2 2020, 7:23 PM