This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [ranges] Simplify our idiom for testing niebloid-ness.
ClosedPublic

Authored by Quuxplusone on Dec 29 2021, 12:01 PM.

Details

Summary

In the test files, replace the old-style tests with a simple static_assert, matching the current style as depicted in e.g. ranges_uninitialized_default_construct.pass.cpp.

Preserve is_function_like (but renamed to is_niebloid) at @ldionne's request. The removal of this test helper will happen in D116570 if at all.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Dec 29 2021, 12:01 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptDec 29 2021, 12:01 PM
ldionne requested changes to this revision.Dec 29 2021, 12:58 PM

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).

This revision now requires changes to proceed.Dec 29 2021, 12:58 PM
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.cpp
18–21

@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."

Add missing "test_macros.h".

ldionne added inline comments.Jan 3 2022, 2:24 PM
libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/special_function.compile.pass.cpp
18–21

(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_asserts for incomplete types inside ranges::begin).

Quuxplusone retitled this revision from [libc++] Eliminate `__function_like`. to [libc++] [ranges] Simplify our idiom for testing niebloid-ness..
Quuxplusone edited the summary of this revision. (Show Details)

Defer the code diff; this is now just the test diffs.

[libc++] [ranges] Simplify our idiom for testing niebloid-ness.

In the test files, replace the old-style tests with a simple static_assert,
matching the current style as depicted in e.g.
`ranges_uninitialized_default_construct.pass.cpp`.
ldionne requested changes to this revision.Jan 4 2022, 6:51 AM
ldionne added inline comments.
libcxx/test/support/test_standard_function.h
1

We should keep this, but make this a libc++ specific helper. We could also rename it is_niebloid.

This revision now requires changes to proceed.Jan 4 2022, 6:51 AM
Quuxplusone edited the summary of this revision. (Show Details)

Update per discussion with @ldionne today. The plan is to keep is_niebloid (née is_function_like) for now; defer its removal until D116570; and make sure to open an LWG issue complaining about the status quo before making the final-final decision on D116570.

If CI is green, imma land this.

ldionne accepted this revision.Jan 4 2022, 12:58 PM
This revision is now accepted and ready to land.Jan 4 2022, 12:58 PM
This revision was landed with ongoing or failed builds.Jan 6 2022, 11:21 AM
This revision was automatically updated to reflect the committed changes.
libcxx/include/module.modulemap