Page MenuHomePhabricator

[clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check
Needs ReviewPublic

Authored by Izaron on Feb 1 2022, 4:01 PM.

Details

Summary

C++17 inline variables are far more convenient compared with
extern const variables declarations, and in general are more ODR-safe and
memory efficient compared with non-inline const variables definitions.
It is a part of a number of code styles to use C++17 inline variables.
The checker would help users to follow this code style.

Diff Detail

Event Timeline

Izaron created this revision.Feb 1 2022, 4:01 PM
Izaron requested review of this revision.Feb 1 2022, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 4:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Izaron added a comment.EditedFeb 1 2022, 4:11 PM

For example, Google Code Style strongly advises C++17 inline variables. There are company guides how to use them: https://abseil.io/tips/168, https://abseil.io/tips/140. I believe other codestyles also use the feature.

P.S. If this review is eventually approved, kindly please merge the commit on my behalf =) As I don't have merge access. My name is Evgeny Shulgin and email is izaronplatz@gmail.com. Sorry for inconvenience!

UPD: I've got merge access, so the paragraph above is irrelevant

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
34

I think this is excessive comment. It's regular convention.

clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
6 ↗(On Diff #405116)

Please synchronize first statement with Release Notes.

53 ↗(On Diff #405116)

Please highlight option values in this section with single back-ticks.

You've probably had this check in development for some time, but
we just updated the documentation for contributing to clang-tidy
and it might help to go over it for your new check to see if there
is anything that stands out.
https://clang.llvm.org/extra/clang-tidy/Contributing.html

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
65–69

Comment: Why don't we have a variadic unless(...) matcher?

Is there any utility to reordering these unless() clauses in
order of "most likely encountered first" to get an early out
when matching?

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
20

This block comment mentions functions, but the documentation only mentions variables.

clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
6 ↗(On Diff #405116)

...const variable definitions...const variable declarations

10 ↗(On Diff #405116)

...variables are a deprecated way...

Izaron updated this revision to Diff 416347.Mar 17 2022, 3:32 PM
Izaron marked 7 inline comments as done.

Fix issues

Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 3:32 PM
Izaron added inline comments.Mar 17 2022, 3:33 PM
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
65–69

Thanks, it is more convenient!

reordering these unless() clauses in order of "most likely encountered first"

I reordered them based on my own approximal estimation. We could make a benchmark to observe the impact of different orders, but it goes beyond this patch.

clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
20

Oops, that's a typo: of course there are nothing abouth functions anywhere in the checker.

You've probably had this check in development for some time, but
we just updated the documentation for contributing to clang-tidy
and it might help to go over it for your new check to see if there
is anything that stands out.
https://clang.llvm.org/extra/clang-tidy/Contributing.html

Thanks, this is excellent documentation! I learned a plenty of new things.

LegalizeAdulthood requested changes to this revision.Jun 22 2022, 3:06 PM

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

  • fold your changes into the appropriate subdirs, stripping the module prefix from new files.
  • make the target check-clang-extra to validate your tests
  • make the target docs-clang-tools-html to validate any docs changes
This revision now requires changes to proceed.Jun 22 2022, 3:06 PM
njames93 added inline comments.Jun 22 2022, 11:21 PM
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
65–69

does unless(isVarInline(), isExternC()) mean unless(isVarInline() && isExternC()) or unless(isVarInline() || isExternC())
This kind of ambiguity could be addressed with unlessAllOf and unlessAnyOf but I don't think its worth the effort.

Izaron updated this revision to Diff 466660.Oct 10 2022, 4:17 PM

Rebased onto main (after a long long time).

Thanks to @LegalizeAdulthood for instructions!

Eugene.Zelenko added inline comments.Oct 10 2022, 4:23 PM
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
6

Excessive newline.

Izaron updated this revision to Diff 466661.Oct 10 2022, 4:28 PM

Removed excessive newline.

Thanks to @Eugene.Zelenko!

Izaron marked an inline comment as done.Oct 10 2022, 4:28 PM

A friendly ping 🙂