[libcxx-commits] [PATCH] D142811: [libcxx][ranges] revert join_view::iterator and sentinel to be in-class

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/join_view.h:184
+  template <class _Tp>
+  struct __is_join_view_iterator_checker{
+    static false_type __check();
----------------
huixie90 wrote:
> ldionne wrote:
> > huixie90 wrote:
> > > philnik wrote:
> > > > This seems quite complicated. Is there a reason not to use `__is_primary_template` instead?
> > > I think the idea is exactly the same (the sfinae technique is also exactly the same but I did not know it is abstracted in _IsValidExpansion). but __is_primary_template does not sound the right name
> > Can't we simply give this a private member called `__is_join_view_iterator` and check whether that member exists from a befriended class?
> @ldionne 
> That was the first thing I tried.
> 
> https://godbolt.org/z/Ws3baE1E1
> 
> It did not work because, even though `Derived` does not have access `Base::flag`. the friend `Checker` can still access `Derived::flag` as the `flag` is part of the `Base` sub object.
> 
> today I realised that I can make the befriended thing a template and only friend the particular instantiation.
> https://godbolt.org/z/771rjzvzq
> 
> what do you think?
To be honest, I am not certain that all of these gymnastics are necessary. I don't see why someone would ever inherit from one of these iterators, and if they do, I think it may be reasonable for them to be broken. And if there's a good reason to do that, then we can change our detection of segmented iterators to account for that as a bug fix. So personally, I'd go for the simplest mechanism that doesn't handle derived types properly and I'd call it a day.

The other thing we could do BTW is mark the iterator class as `final` (which is non-conforming but not by much, and for an arguably good reason), and/or create a LWG issue (or write a paper?) asking for ranges iterators to be marked `final`. Ranges iterators are somewhat special because they are defined as nested classes, which is not super frequent and creates the ADL difficulties that you noted in the first place. As a result, I think it would make sense to consider this problem for all nested iterator types inside Ranges and I think a paper would probably make sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142811