[libcxx-commits] [PATCH] D118668: [libc++][NFC] Add namespace comments in ranges

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__ranges/reverse_view.h:117
     template<class _Tp>
     constexpr bool __is_reverse_view<reverse_view<_Tp>> = true;
 
----------------
Quuxplusone wrote:
> ldionne wrote:
> > philnik wrote:
> > > Here, in line 123 and line 129 clang-tidy claims these definitions can lead to ODR-violations. Is inline implied by it being a template or something else and therefore this is a clang-tidy bug or is this actually potentially an ODR-violation and this is a libc++ bug?
> > I'll use cppreference instead of the standard here because everything I found in the Standard was much harder to understand than cppreference.
> > 
> > My understanding is that the `__is_reverse_view` base template has the usual linkage for templates, which is `extern inline` (more precisely linkonce_odr in LLVM lingo). See **external linkage** at https://en.cppreference.com/w/cpp/language/storage_duration.
> > 
> > However, the `__is_reverse_view<reverse_view<_Tp>>` specialization is a variable, not a variable template, so this part of https://en.cppreference.com/w/cpp/language/storage_duration / **internal linkage** applies to it instead:
> > 
> > > non-volatile non-template non-inline non-exported const-qualified variables (including constexpr) that aren't declared extern and aren't previously declared to have external linkage;
> > 
> > Naively, this would mean that `__is_reverse_view<reverse_view<_Tp>>` has internal linkage, and hence `clang-tidy` is right to flag this.
> > 
> > However, according to https://stackoverflow.com/a/48078494/627587, it appears that the rule doesn't actually apply, and so `clang-tidy` would in fact be wrong to flag this. Perhaps `clang-tidy` doesn't implement the resolution of CWG1713?
> > 
> > Also, one way to know for sure would be to look at the LLVM bitcode emitted, and confirm that `__is_reverse_view<reverse_view<_Tp>>` has linkoncde_odr linkage. If so, then `clang-tidy` would definitely be wrong to flag this.
> My understanding is that the variables instantiated from these templates behave the same as any other variables instantiated from templates — they shouldn't need `inline` for correctness, any more than a variable template (or function template) needs `inline` for correctness.
> However, //for consistency with the entire rest of the library//, I think it would be a good idea to make these `inline constexpr bool` throughout. The STL uses `inline constexpr bool` for its own type-trait variables; even if they had no reason to do that originally, we certainly have no reason to innovate here. (Economist's $100 Bill blog post. :))
> So I'm in favor of adding `inline`.
> 
> Also, in `__ranges/join_view.h`, I would change
> `constexpr bool __use_const =` to `static constexpr bool _UseConst =` for consistency with some superficially similar code in `subrange` and `reverse_view`.
This is now #53519. I'll make a PR later to add inline to the constexpr variables.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118668/new/

https://reviews.llvm.org/D118668