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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
