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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:180
+class CE {
+  constexpr static int i = 5; // OK: constexpr definition.
+};
----------------
xazax.hun wrote:
> hokein wrote:
> > hokein wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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.
> > > 
> > >   
> > 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). 
> Unfortunately, the test will fail without this modification, but I modified it to ignore inline variables, which is a way better approach indeed. 
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D34449