Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use clang-tidy #561

Open
GPMueller opened this issue Apr 2, 2020 · 4 comments
Open

Use clang-tidy #561

GPMueller opened this issue Apr 2, 2020 · 4 comments
Labels
idea

Comments

@GPMueller
Copy link
Member

GPMueller commented Apr 2, 2020 •
edited
Loading

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 GPMueller added the idea label Apr 2, 2020
@GPMueller
Copy link
Member Author

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
Copy link
Member Author

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 GPMueller reopened this Sep 25, 2021
@GPMueller
Copy link
Member Author

GPMueller commented Sep 25, 2021 •
edited
Loading

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
Copy link
Member Author

GPMueller commented Nov 6, 2021 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
idea
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@GPMueller