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.

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

[CI] Add a clang-tidy build #2051

Closed
marcalff opened this issue Mar 14, 2023 · 4 comments · Fixed by #3001
Closed

[CI] Add a clang-tidy build #2051

marcalff opened this issue Mar 14, 2023 · 4 comments · Fixed by #3001
Labels
do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers

Comments

@marcalff
Copy link
Member

Add a clang-tidy build in CI.

@marcalff marcalff added help wanted Good for taking. Extra help will be provided by maintainers good first issue Good for newcomers labels Mar 14, 2023
@esigo
Copy link
Member

esigo commented Mar 27, 2023

depends on #2053.

@github-actions
Copy link

This issue was marked as stale due to lack of activity.

@chusitoo
Copy link
Contributor

chusitoo commented Dec 22, 2023

I started looking into adding clang-tidy to the build, subject to first addressing issues found in cleanup #2053, which I also wanted to tackle to sort of get more acquainted with the rest of the codebase.

If all checks are enabled, the list of smells can get overwhelming very quickly; from the full list found at https://clang.llvm.org/extra/clang-tidy/checks/list.html, most of the cppcoreguidelines-* are a no-brainer IMHO.
It might be worthwhile to also add performance-* and bugprone-*, although even then there would be some more fundamental issues related to things like methods marked noexcept that themselves rely on functions that throw (i.e. trace_api::IsRootSpan calling std::get(std::variant) without catching std::bad_variant_access).

In fact, even cppcoreguidelines might need to be fune-tuned, as just running clang-tidy against api/include/opentelemetry/ produced 370 warnings, about half of them related to macro usage and magic numbers.

Ultimately, in order to narrow down the scope of the cleanup that needs to be performed, a subset of checks to run against would have to be established first. It could always start as a very specific set of rules and expand the checks later on.

@marcalff
Copy link
Member Author

marcalff commented Jul 7, 2024

CI was implemented recently for include-what-you-use, see:

In my understanding, both iwyu and clang-tidy rely on CMake and a compilation database, so it is likely that the CI for clang-tidy can be implemented following the include-what-you-use pattern.

Desirable features for the CI:

  • Leverage CMAKE options WITH_XYZ, to control which code (OTLP_HTTP, etc) is covered
  • Do not fail on the first error or warning, collect as much as possible (i.e., make -k)
  • Collect all warnings in a log, and upload the log as artifact, so it can be inspected
  • Count the number of remaining warnings. This is to enforce a build pass/fail rule later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-stale good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants