[libcxx-commits] [PATCH] D116384: [libc++] Eliminate `__function_like`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.cpp:16-19
+// Because this is a variable and not a function, it's guaranteed that ADL won't be used. However,
+// implementations are allowed to use a different mechanism to achieve this effect, so this check is
+// libc++-specific.
+LIBCPP_STATIC_ASSERT(std::is_class_v<decltype(std::ranges::advance)>);
----------------
Quuxplusone wrote:
> @ldionne wrote:
> > The testing simplifications make sense to me, but removing `__function_like` does mean that we weaken our implementation w.r.t. Hyrum's law. I don't remember coming to a consensus on removing that part (and I would most likely object to that).
> 
> (1) It's physically possible to land the libcxx/test/ changes without the libcxx/include/ changes. Shall I do that? (and then leave this PR open for discussion of the libcxx/include/ changes)
> 
> (2) Re Hyrum's Law: You have to keep in mind that "there's no such thing as an archetype." C++ has only leaky abstractions. Here we have to choose, for each niebloid in the library, should it be copyable or non-copyable? final or non-final? default-initializable or non-default-initializable? We cannot avoid choosing one or the other. Whichever one we choose, the user //might// (accidentally or on purpose) end up relying on it. They'll have some template that does one thing if the type is copyable and a different thing if it's not, and then boom, technically we can't change that without an ABI break.
> The paper standard saves us here by making most of those problematic things IFNDR, so we actually //can// change this stuff without worrying about the breakage to users. Therefore I think our goal should be simplicity and predictability: we should do the simplest thing that works, because complexity without reason is bad.
> 
> FWIW, today, GCC accepts and MSVC rejects https://godbolt.org/z/hhae7zEf9
> ```
>     std::ranges::transform(v, w.begin(), std::ranges::next);
> ```
> Everyone (including libc++) accepts things like
> ```
>     (void) std::identity()(std::ranges::next);
>     std::optional<decltype(std::ranges::next)> o;
>     std::function<int*(int*)> f = std::ref(std::ranges::next);
> ```
> These are all IFNDR as far as I'm aware: the user is supposed to know that `std::ranges::next` (despite being a niebloid) isn't //supposed// to be used in these ways. This is basically analogous to the stuff that appears in SD-8: "sure you can try doing this, but the library vendor doesn't have to coddle you and you're on your own if it breaks later."
> (1) It's physically possible to land the libcxx/test/ changes without the libcxx/include/ changes. Shall I do that? (and then leave this PR open for discussion of the libcxx/include/ changes)

Yes, please. Ideally, this review could contain the testing changes only and then we can argue on another review whether to keep `__function_like` or not (see below).


> (2) Re Hyrum's Law: [...]

I understand your point, and I agree that `__function_like` isn't perfect. However, in my opinion, the comment in `function_like.h` summarizes it nicely:

> // Since these are still standard library functions, we use `__function_like` to eliminate most of
> // the properties that function objects get by default (e.g. semiregularity, addressability), to
> // limit the surface area of the unintended public interface, so as to curb the effect of Hyrum's
> // law.

The goal isn't to be perfect, it is to limit the API surface that users can unknowingly start depending on. If that leads to too much implementation divergence on properties of those niebloids (e.g. libc++ isn't copyable but libstdc++ is), and if all implementations end up using classes to implement niebloids, then the Standard should just say it and not make it IFNDR. But weakening our implementation's strictness is not something I support unless it has a strong technical benefit associated (like your other patch for removing `static_assert`s for incomplete types inside `ranges::begin`).



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

https://reviews.llvm.org/D116384