[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.
+};
----------------
hokein wrote:
> aaron.ballman wrote:
> > 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.
> > 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?
> 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?


Repository:
  rL LLVM

https://reviews.llvm.org/D34449