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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

The patch itself LGTM, but there's indeed an interesting side question about the linkage of variable templates.



================
Comment at: libcxx/include/__ranges/reverse_view.h:117
     template<class _Tp>
     constexpr bool __is_reverse_view<reverse_view<_Tp>> = true;
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118668