Page MenuHomePhabricator

[clang-tidy] Enable inline variable definitions in headers.
ClosedPublic

Authored by xazax.hun on Jun 21 2017, 7:03 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun created this revision.Jun 21 2017, 7:03 AM
xazax.hun retitled this revision from [Clang-tidy] Enable constexpr definitions in headers. to [clang-tidy] Enable constexpr definitions in headers. .Jun 21 2017, 7:03 AM
aaron.ballman added inline comments.Jun 21 2017, 7:35 AM
test/clang-tidy/misc-definitions-in-headers.hpp
180 ↗(On Diff #103371)

This is not as safe as you might think. As-is, this is fine, however, if the class is given an inline function where that variable is odr-used, you will get an ODR violation.

I think it's mildly better to err on the side of safety here and diagnose.

hokein added inline comments.Jun 21 2017, 8:32 AM
test/clang-tidy/misc-definitions-in-headers.hpp
1 ↗(On Diff #103371)

The original code should work as -std=c++11 will be added defaultly by check_clang_tidy script.

180 ↗(On Diff #103371)

I think the current code (Line 97 in DefinitionsInHeadersCheck.cpp) has already guaranteed this case. Can you try to run it without your change in the DefinitionsInHeadersCheck.cpp?

I think it still makes sense to add constexpr test cases.

hokein edited edge metadata.Jun 22 2017, 12:59 PM

Have thought this a bit more, I misunderstood your patch previously (sorry for that).

I think what you intend to do is to ignore C++17 inline variables in headers, am I correct?

test/clang-tidy/misc-definitions-in-headers.hpp
1 ↗(On Diff #103371)

constexpr variables have internal linkage, which should be detected for the current check (but the test case is missing this kind of case).

If you want to test inline variables, I'd suggest adding a new test file like misc-definitions-in-headers-1z.hpp which includes cases of inline variables.

180 ↗(On Diff #103371)

In C++17, constexpr static int i is an inline variable, which is fine to define in C++ header -- because inline specifier provides a facility allowing definitions (functions/variables) in header that is included in multiple TUs. Additionally, one of the inline variable motivations is to support the development of header-only libraries, you can find discussions in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4424.pdf.

Therefore, I'm +1 ignore the inline variables (the same as inline functions).

xazax.hun updated this revision to Diff 104111.Jun 27 2017, 2:27 AM
xazax.hun marked 5 inline comments as done.
  • Updates according to the reviews.

BTW, could you please also update the docs/clang-tidy/checks/misc-definitions-in-headers.rst?

test/clang-tidy/misc-definitions-in-headers-1z.hpp
5 ↗(On Diff #104111)

Could you follow the way of comment in the misc-definitions-in-headers-1z.hpp? For each allowed case, add the comment like constexpr static int i = 5; // OK: inline variable definition.

The same below.

10 ↗(On Diff #104111)

I think this case is allowed because of internal linkage. I'd suggesting putting it to misc-definitions-in-headers.hpp.

xazax.hun updated this revision to Diff 104360.Jun 28 2017, 1:50 AM
xazax.hun marked 2 inline comments as done.
xazax.hun retitled this revision from [clang-tidy] Enable constexpr definitions in headers. to [clang-tidy] Enable inline variable definitions in headers. .
xazax.hun edited the summary of this revision. (Show Details)
  • Updates according to the review comments.
xazax.hun updated this revision to Diff 104379.Jun 28 2017, 3:37 AM
  • Unbreak the constexpr test.
test/clang-tidy/misc-definitions-in-headers.hpp
1 ↗(On Diff #103371)

It looks like -std=c++11 is not added.

180 ↗(On Diff #103371)

Unfortunately, the test will fail without this modification, but I modified it to ignore inline variables, which is a way better approach indeed.

hokein accepted this revision.Jun 28 2017, 4:30 AM

Looks good, thanks!

test/clang-tidy/misc-definitions-in-headers.hpp
1 ↗(On Diff #103371)

OK. Yes, you are right.

The -std=c++11 only be added if the test file extension is cpp. I think we can extend the check_clang_tidy.py script to support it, but it can be done in a follow-up.

This revision is now accepted and ready to land.Jun 28 2017, 4:30 AM
This revision was automatically updated to reflect the committed changes.
aaron.ballman added inline comments.Jun 28 2017, 6:09 AM
test/clang-tidy/misc-definitions-in-headers.hpp
180 ↗(On Diff #103371)

The paper you cited was a feature not a defect, so prior to the paper's adoption in C++17, the behavior is that the constexpr variable may trigger an ODR violation, which is why I was saying this should be diagnosed rather than ignored. There was real-world motivation for that paper.

hokein added inline comments.Jun 28 2017, 12:06 PM
test/clang-tidy/misc-definitions-in-headers.hpp
180 ↗(On Diff #103371)

prior to the paper's adoption in C++17, the behavior is that the constexpr variable may trigger an ODR violation

Yeah, I did remember our discussion when I implemented this check (https://stackoverflow.com/questions/23652156/how-would-use-of-unnamed-namespaces-in-headers-cause-odr-violations). We allow internal linkage variables (static/const/conexpr) in the check -- because we want to keep a small number of warnings as const variable definitions are widely used in headers.

Maybe add an option to enable this particular case?

aaron.ballman added inline comments.Jun 28 2017, 12:31 PM
test/clang-tidy/misc-definitions-in-headers.hpp
180 ↗(On Diff #103371)

Maybe add an option to enable this particular case?

An option would make sense to me. Perhaps the default value of the option can even be set depending on the language standard used to run the compilation?