[libcxx-commits] [PATCH] D101922: [libcxx][iterator] adds `std::ranges::advance`

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:18-19
   __iterator/iterator_traits.h
+  __iterator/functionish.h
+  __iterator/primitives.h
   __iterator/readable_traits.h
----------------
Both of these filenames violate my continuing-to-devolve-but-still-fairly-conservative rules about helper header naming. Looking at these names I have no idea what these headers contain or for what purpose they exist as separate entities.


================
Comment at: libcxx/include/__iterator/functionish.h:31
+//
+// Since these are still standard library functions, we use `__functionish` to eliminate most of
+// the properties that function objects get by default (e.g. semiregularity, addressability), to
----------------
cjdb wrote:
> Mordante wrote:
> > Can we use a clearer name, for example `__function_like`, `ish` sounds rather fishy to me. Or maybe something with ADL in the name, but I've no nice name for that to offer.
> `__function_like` wfm. There's more going on here than just ADL inhibition, so we'd need a very good replacement.
I believe the traditional name is "niebloid"; could we use "niebloid"? (Or point to a technical difficulty with "niebloid"?)
In that case, this class would be `niebloid_base`, and the filename would be `__ranges/niebloid_base.h` (`__iterator/`? `__algorithm/`?)

But at a higher level, I don't see why we'd want all of our niebloids to inherit from a common base class at all. This seems like the sort of thing that should be handled via a macro, so that we could just write, like,
```
class __advance_fn {
    _LIBCPP_NIEBLOID_SPECIAL_MEMBERS(__advance_fn);
public:
    void operator()(...) const...
    etc...
};
```
You're also trying to [command the tides](https://en.wikipedia.org/wiki/King_Canute_and_the_tide) a little bit here; e.g. you're going out of your way to make `__advance_fn` "not default constructible," but that doesn't actually stop anyone from writing e.g. `decltype(std::ranges::advance) ha = {};`.
We also //need// people to be able to pass these objects around like functions, right? I mean,
```
std::vector<int*> v = ...;
std::transform(v.begin(), v.end(), std::ranges::next);
```
is supposed to work? (Right?) If we need that to work, then making these objects immovable is actually counterproductive: we want them to be copyable just like function pointers and lambdas would be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922