Use clang-tidy - Githubissues

GPMueller commented 4 years ago

clang-tidy is a useful tool for helping to keep a good code quality. I would suggest looking into the following option classes:

bugprone-
clang-analyzer-
cppcoreguidelines-
modernize-
openmp-
performance-
portability-
readability-

It can give warnings, but with -fix or --fix-errors it can even automatically apply some corrections.

The full list of options is here.

Notably, it also integrates well with IDEs (see here and e.g. this vs-code extension).

From a terminal,

use -DCMAKE_EXPORT_COMPILE_COMMANDS=ON when calling cmake (e.g. add it to the cmake.sh)
use -p=build when calling clang-tidy, so it will know all include directories, defines etc.

GPMueller commented 4 years ago

After testing it a little, I have the impression it would be better to first activate all compiler warnings and work through those. Then one could go through all the clang-tidy options and see which ones really make sense here. For example in this code base, warning about "pointer arithmetic", e.g. when using operator[], does not make sense...

The config I was testing so far:

---
Checks: 'bugprone-*,
         clang-analyzer-*,
         clang-diagnostic-*,
         cppcoreguidelines-*,
         misc-*,
         modernize-*,
         openmp-*,
         performance-*,
         portability-*,
         readability-*,
         -cppcoreguidelines-pro-bounds-pointer-arithmetic,
         -modernize-pass-by-value,
         -modernize-return-braced-init-list,
         -modernize-use-trailing-return-type,
         -readability-braces-around-statements'

GPMueller commented 4 years ago

I'm actually not so convinced of this anymore. While trying around with it, it did not seem to help more with the Spirit codebase than compiler warnings.

GPMueller commented 2 years ago

Actually, due to the fact that clang-tidy can provide IDE support (e.g. via clangd, which has a vs code extension) and corresponding auto-fixes and refactorings, it would in fact be quite useful. An added benefit, complementary to clang-format (see issue #550), would be the ability to automatically check given style- and naming-convention, e.g.

---
WarningsAsErrors: ''
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle: file
Checks: >
  -*,
  readability-identifier-naming
  readability-braces-around-statements
  readability-function-size
CheckOptions:
  - { key: readability-braces-around-statements.ShortStatementLines,        value: '1'          }
  - { key: readability-function-size.StatementThreshold,                    value: '800'        }
  - { key: readability-function-size.LineThreshold,                         value: '200'        }
  - { key: readability-identifier-naming.NamespaceCase,                     value: 'lower_case' }
  - { key: readability-identifier-naming.EnumCase,                          value: 'CamelCase'  }
  - { key: readability-identifier-naming.ClassCase,                         value: 'CamelCase'  }
  - { key: readability-identifier-naming.StructCase,                        value: 'CamelCase'  }
  - { key: readability-identifier-naming.AggressiveDependentMemberLookup,   value: '1'          }
  - { key: readability-identifier-naming.PrivateMemberSuffix,               value: '_'          }
  - { key: readability-identifier-naming.MethodCase,                        value: 'lower_case' }
  - { key: readability-identifier-naming.FunctionCase,                      value: 'lower_case' }
  - { key: readability-identifier-naming.MemberCase,                        value: 'lower_case' }
  - { key: readability-identifier-naming.VariableCase,                      value: 'lower_case' }
  - { key: readability-identifier-naming.ParameterCase,                     value: 'lower_case' }
  - { key: readability-identifier-naming.GlobalConstantCase,                value: 'UPPER_CASE' }
  - { key: readability-identifier-naming.StaticConstantCase,                value: 'UPPER_CASE' }
...

Note, compile commands should be exported by default in order to provide a more automatic IDE integration. We could simply add set(CMAKE_EXPORT_COMPILE_COMMANDS ON) to the root CMakeLists.txt, see https://stackoverflow.com/a/45781334/4069571.

GPMueller commented 2 years ago

Activating all the checks in my previous comment is definitely overkill. However, there are plenty of checks that have a high probability to be pointing to an actual bug or the adherence to which may prevent an array of problems. I believe the following would be useful without making a lot of unnecessary noise:

bugprone

SEI CERT C and C++ guidelines

C++ core guidelines

high integrity C++ guidelines

misc

modernize

OpenMP

performance

readability

clang static analyzer

spirit-code / spirit

Use clang-tidy #561