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

Thu May 6 10:12:00 PDT 2021

cjdb added a comment.

> Is the synopsis up to date?

It is now :-)

================
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:
> Quuxplusone wrote:
> > 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.
> > I believe the traditional name is "niebloid"; could we use "niebloid"? (Or point to a technical difficulty with "niebloid"?)
> 
> `advance` is a "niebloid", but this `__function_like` has no relationship with niebloids. I'm just getting back the functionality of functions here. Also, "niebloid" is a cute name, but it's about as informative as "Koenig" lookup (so I'd prefer a more boring name). As for where this file lives, I have zero opinion.
> 
> > 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,
> 
> I'm certainly not married to the current implementation: whatever's most readable is probably best (readable for the user getting a diagnostic, that is). Lemme experiment when I am working on this next.
> 
> > You're also trying to command the tides 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 = {};`.
> 
> Is that because it's an aggregate? //Ugh//.
> 
> > 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.
> 
> Should it work? Morally, yes. Unless this is an addressable function (and I don't see that it is), the standard goes out of its way to say that this is //not// possible (grep for "addressable" in http://wg21.link/library). [[ https://godbolt.org/z/sbja7dTfe | Try doing the above with std::next ]], for example. Niebloids are not function objects: it's just the easiest (and only portable) way we can do them presently.
> You're also trying to command the tides 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 = {};`.

I just tried this and it's not valid. Are there any other ways it can be written?

================
Comment at: libcxx/include/__iterator/primitives.h:39
+  [[nodiscard]] static constexpr auto __abs(_Tp const __n) noexcept {
+    _LIBCPP_ASSERT(__n != numeric_limits<_Tp>::min(), "`-numeric_limits<I>::min()` is undefined");
+    return __n < 0 ? -__n : __n;
----------------
Mordante wrote:
> Can't we return the unsigned version of `_Tp`? Then  we can return the largest negative value.
I suppose we could. I don't like treating unsigned ints as "non-negative whole numbers", but I don't think we've got a choice here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922