power-grid-model icon indicating copy to clipboard operation
power-grid-model copied to clipboard

Resolve code smells from `clang-tidy` on C++ side

Open mgovers opened this issue 1 year ago • 1 comments

The clang-tidy action checks for static analyzer warnings (similarly to SonarCube). Currently, there are quite some checks that are disabled in the .clang-tidy file. Disabled warnings are prefixed - in the .clang-tidy file, e.g. -llvm-else-after-return disables the llvm-else-after-return check.

Note that some checks are more easily fixable than others! If you get stuck on one check, just move on to the next one! Check the list of easily fixable checks below.

Desired solution

  • Create a branch
  • Remove one disabled check from the list of easily fixable checks below
    • Check the checkbox on the error you fixed below
  • Compile using one of the CMake Presets with clang-tidy in the name, as explained in the Documentation, or wait for CI to fail on the given check.
  • Fix the clang-tidy errors
    • Reasonably good explanations for each error exist in the LLVM docs (just do a Google search on the name of the error).
    • Sometimes, the clang-tidy error message and following notes provide suggestions on how to fix the issue. Most of the times, these proposed changes are good, but beware about applying them blindly, as sometimes they fail to consider the greater picture. Always double-check
  • Create a pull request

It is good practice to fix only one check per pull request to keep the amount of changes manageable.

Compilation of checks

As mentioned above, some clang-tidy checks may be hard to fix, especially for people that are relatively new to C++. Here is a list of good starting points. Again, if you get stuck, don't focus too long on one check but just move on to the next one. There are plenty low hanging fruits left.

Suggestions for easily fixable checks

  • cppcoreguidelines-* checks are important but sometimes hard to resolve. Here are some of the few exceptions that may be easy regardless:`
    • [x] -cppcoreguidelines-narrowing-conversions
    • [x] -cppcoreguidelines-prefer-member-initializer
    • [x] -cppcoreguidelines-special-member-functions
  • performance-* checks are almost always useful and easy to fix!
    • [x] -performance-no-automatic-move
    • [x] -performance-unnecessary-copy-initialization
  • bugprone-* checks help write code better, faster
    • [x] -bugprone-macro-parentheses
    • [x] -bugprone-narrowing-conversions
  • misc-* are checks that do not really fit into any of the other checks topics. Most are easy to solve.
    • [x] -misc-const-correctness
  • most readability-* checks are fairly easy to solve
    • [x] -readability-braces-around-statements
    • [x] -readability-container-size-empty
    • [x] -readability-convert-member-functions-to-static
    • [x] -readability-else-after-return
    • [x] -readability-implicit-bool-conversion
    • [x] -readability-isolate-declaration
    • [x] -readability-make-member-function-const
    • [x] -readability-named-parameter
    • [x] -readability-redundant-access-specifiers
    • [x] -readability-redundant-member-init
    • [x] -readability-suspicious-call-argument
  • some modernize-* checks are easy to resolve
    • [x] -modernize-concat-nested-namespaces
    • [x] -modernize-deprecated-headers
    • [ ] -modernize-loop-convert
    • [x] -modernize-pass-by-value
    • [x] -modernize-use-auto
    • [x] -modernize-use-default-member-init
    • [x] -modernize-use-emplace
    • [x] -modernize-use-transparent-functors
    • [x] -modernize-use-using

Known hard-to-fix checks

Below are a couple of issues that are hard to fix, may take some time, or require more extensive C++ knowledge.

  • cppcoreguidelines-*
    • [ ] -cppcoreguidelines-avoid-c-arrays
    • [ ] -cppcoreguidelines-avoid-magic-numbers
    • [ ] -cppcoreguidelines-init-variables
    • [ ] -cppcoreguidelines-macro-usage
    • [ ] -cppcoreguidelines-no-malloc
    • [ ] -cppcoreguidelines-narrowing-conversions
    • [ ] -cppcoreguidelines-non-private-member-variables-in-classes
    • [ ] -cppcoreguidelines-owning-memory
    • [ ] -cppcoreguidelines-pro-bounds-constant-array-index
    • [ ] -cppcoreguidelines-pro-type-cstyle-cast
    • [ ] -cppcoreguidelines-pro-type-member-init
    • [ ] -cppcoreguidelines-pro-bounds-pointer-arithmetic
    • [ ] -cppcoreguidelines-pro-type-reinterpret-cast
  • bugprone-*
    • [ ] -bugprone-easily-swappable-parameters
    • [ ] -bugprone-exception-escape
  • misc-*
    • [ ] -misc-no-recursion
    • [ ] -misc-non-private-member-variables-in-classes
    • [ ] -misc-include-header-cleaner
  • readability-*
    • [ ] -readability-function-cognitive-complexity
    • [ ] -readability-identifier-length
    • [ ] -readability-magic-numbers
  • modernize-*
    • [ ] -modernize-avoid-bind
    • [ ] -modernize-avoid-c-arrays
    • ~[ ] -modernize-use-nodiscard~ (we decided not to introduce this into the code due to its verbosity)
    • [ ] -modernize-use-trailing-return-type

mgovers avatar Jun 08 '23 07:06 mgovers

I removed the good-first-issue label because all easy-to-fix things were fixed and merged. the remaining issues require more extensive knowledge and/or a discussion about our own opinion on the smells

mgovers avatar Nov 10 '23 07:11 mgovers