Decide on which clang-tidy checks we want to enable

[psalz]

I've parsed all clang-tidy diagnostics generated with the following checks enabled:

-*,
bugprone-*,
clang-analyzer-*,
clang-diagnostic-*,
cppcoreguidelines-*,
mpi-*,
performance-*,
readability-*

and those are the results:

Found 2137 diagnostics (out of 13487 parsed; 11350 were duplicates). 
  787: cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers 
  678: readability-identifier-length
  104: readability-function-cognitive-complexity
   92: readability-qualified-auto
   41: cppcoreguidelines-avoid-c-arrays
   37: cppcoreguidelines-pro-bounds-pointer-arithmetic
   36: cppcoreguidelines-init-variables
   30: cppcoreguidelines-special-member-functions
   23: bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions
   22: cppcoreguidelines-macro-usage
   22: performance-unnecessary-value-param
   19: cppcoreguidelines-avoid-non-const-global-variables
   17: readability-implicit-bool-conversion
   16: readability-isolate-declaration
   16: readability-named-parameter
   16: bugprone-macro-parentheses
   15: bugprone-easily-swappable-parameters
   14: cppcoreguidelines-pro-bounds-constant-array-index
   12: bugprone-exception-escape
   12: cppcoreguidelines-pro-bounds-array-to-pointer-decay
   11: cppcoreguidelines-pro-type-member-init
   10: clang-analyzer-deadcode.DeadStores
   10: bugprone-implicit-widening-of-multiplication-result
   10: cppcoreguidelines-pro-type-static-cast-downcast
    7: cppcoreguidelines-pro-type-vararg
    7: performance-move-const-arg
    7: cppcoreguidelines-non-private-member-variables-in-classes
    6: readability-braces-around-statements
    6: readability-else-after-return
    6: cppcoreguidelines-pro-type-reinterpret-cast
    4: readability-redundant-access-specifiers
    4: cppcoreguidelines-owning-memory
    4: readability-avoid-const-params-in-decls
    3: clang-analyzer-core.UndefinedBinaryOperatorResult
    3: performance-faster-string-find
    3: cppcoreguidelines-pro-type-const-cast
    2: readability-simplify-boolean-expr
    2: performance-unnecessary-copy-initialization
    2: performance-for-range-copy
    2: readability-container-size-empty
    2: readability-inconsistent-declaration-parameter-name
    2: readability-static-accessed-through-instance
    2: clang-analyzer-optin.mpi.MPI-Checker
    2: performance-noexcept-move-constructor
    2: bugprone-lambda-function-name
    2: readability-convert-member-functions-to-static
    2: clang-diagnostic-error
    1: performance-inefficient-vector-operation
    1: readability-suspicious-call-argument
    1: clang-analyzer-core.CallAndMessage
    1: readability-make-member-function-const
    1: cppcoreguidelines-prefer-member-initializer

You can find the raw clang-tidy output here (useful for checking what some of these diagnostics even do).

Based on this, I would suggest the following configuration:

-*,
bugprone-*,
clang-analyzer-*,
clang-diagnostic-*,
cppcoreguidelines-*,
-cppcoreguidelines-avoid-c-arrays,
-cppcoreguidelines-macro-usage,
-cppcoreguidelines-pro-bounds-pointer-arithmetic,
mpi-*,
performance-*,
readability-*,
-readability-avoid-const-params-in-decls
-readability-identifier-length,
-readability-magic-numbers,
-readability-uppercase-literal-suffix,

Thoughts?

[fknorr]

My list of personal grievances (to be continued):

• clang-analyzer-optin.mpi.MPI-Checker: We have little explicit MPI code and for that this option generates false positives basically for every single MPI calll (see e.g. #252)
  • misc-misplaced-const lints against const pointer_t function parameters because they expand to pointer_target *const (which is exactly what I wanted)
  • cppcoreguidelines-pro-bounds-constant-array-index lints against things like for (int d=0; d<Dims; d++) range[d] = 1 because the subscript is not a constant expression. No way to avoid this in a template<int Dims>.
  • readability-else-after-return seems to be the exact opposite of the school of "every function should only have a single return statement", but equally misguided. I often want to have an if-else cascade where every branch returns from the function, because the first check is not an early-return but branches have equal weight.
  • cppcoreguidelines-pro-type-const-cast and cppcoreguidelines-avoid-goto: I feel like we know our language well enough so when we end up using one of these, we know exactly what we are doing.
  • readability-convert-member-functions-to-static is frequently emitted for concept implementations such as std::hash<T>::operator(). I have never encountered a case where this ended up being helpful. Sometimes I deliberately make functions non-static because that's the API I want to present to a user.
  • cppcoreguidelines-avoid-do-while: Why? Not that I write these frequently, but they do have valid uses, especially inside macro definitions.
  • bugprone-unchecked-optional-access generates too many false positives (e.g. if has_value was part of a loop condition, or when .value() which is a checked access is used without a conditional)

Additionally, in tests:

• cppcoreguidelines-avoid-non-const-global-variables triggered by Catch2
• cppcoreguidelines-avoid-do-while triggered by Catch2
• misc-use-anonymous-namespace triggered by Catch2 (also anonymous namespaces are as stupid of a concept as statics)