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

[xazax.hun]

Summary

Inline variable definitions should be ok in headers.

https://stackoverflow.com/questions/34445336/constexpr-global-constants-in-a-header-file-and-odr

[aaron.ballman]

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]

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]

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]

• Updates according to the reviews.

[hokein]

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

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

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

[xazax.hun]

• Updates according to the review comments.

[xazax.hun]

• 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]

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.

[aaron.ballman]

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]

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]

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?

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {
     if (VD->getDeclContext()->isDependentContext() && VD->isStaticDataMember())
       return;
     // Ignore instantiated static data members of classes.
     if (isTemplateInstantiation(VD->getTemplateSpecializationKind()))
       return;
     // Ignore variable definition within function scope.
     if (VD->hasLocalStorage() || VD->isStaticLocal())
       return;
+    // Ignore inline variables.
+    if (VD->isInline())
+      return;
 
     diag(VD->getLocation(),
          "variable %0 defined in a header file; "
          "variable definitions in header files can lead to ODR violations")
         << VD;
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-definitions-in-headers-1z.hpp

+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- -- -std=c++1z
+
+// Inline variables should be ok.
+class CE {
+  constexpr static int i = 5;
+};
+
+inline int i = 5;
+
+constexpr int a = 1;
+
+int b = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'b' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]