hokein added inline comments.
================
Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:180
+class CE {
+ constexpr static int i = 5; // OK: constexpr definition.
+};
----------------
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?
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
