Page MenuHomePhabricator

[libcxx][ranges] Add ranges::empty CPO.
ClosedPublic

Authored by cjdb on Apr 23 2021, 12:35 PM.

Details

Summary

Depends on D101079. Refs D101189.

Diff Detail

Event Timeline

zoecarver created this revision.Apr 23 2021, 12:35 PM
zoecarver requested review of this revision.Apr 23 2021, 12:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2021, 12:35 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver added inline comments.Apr 23 2021, 12:36 PM
libcxx/include/__ranges/empty.h
3

Change this here and in other patches.

Quuxplusone added inline comments.Apr 23 2021, 1:10 PM
libcxx/include/CMakeLists.txt
41–42

At some point we should alphabetize end vs ena.

libcxx/include/__ranges/empty.h
49

Doesn't clearly implement a case corresponding to the bullet point ["If T is an array of unknown bound, ranges​::​empty(E) is ill-formed."](https://eel.is/c++draft/range.access#range.prim.empty-2.1)
But I think that bullet point is actually redundant; arrays of unknown bound also don't have ranges::end(E) or ranges::size(E) or t.begin().
Do we understand the intent (in all these CPOs) when T is a reference to an array of unknown bound?

51–54

As usual, replace X with bool(X) in the noexcept-spec and also in the return. Also, __t should be an lvalue according to "let t be an lvalue that denotes the reified object for E." So:

[[nodiscard]] constexpr bool operator()(_Tp&& __t) const
    noexcept(noexcept(bool(__t.empty()))) {
  return __t.empty();
}

Optionally, figure out a way to detect the difference here and write a regression test for it. (I say "optionally" because IMHO it doesn't really matter.)

As above so on lines 34 and 57/58: t is an lvalue, lose the forwarding.

64–65
libcxx/include/ranges
80–83

At some point, alphabetize end and ena.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
25–30

(A) If you're intending this to be found (or rather not found) by ADL, please make it a hidden friend.
(B) Please make the functions look as much like "realistic/simple code" as you can. In this case, that means marking empty() and size() as const-qualified member functions throughout. E.g.:

struct HasMemberAndFunction {
    constexpr bool empty() const { return true; ]
    friend bool empty(const HasMemberAndFunction&) { return false; }
};

And normally I'd say "no constexpr"; but here you need them to be constexpr because you're testing the constexpr-callableness of ranges::empty.

Btw, if you ever run into a situation where you need to test for sure that some specific function was entered, you can use the old "was_called = true;" trick. You just need to make bool& was_called a data member, and initialize it in the constructor to refer to a local variable of the function doing the testing. (Global variables can't be modified in constexpr contexts, but locals can.)

Finally, please add a test that intentionally has a non-const size and empty:

struct S { int size(); bool empty(); };
static_assert(!requires (const S& s) { std::ranges::size(s); });
static_assert(!requires (const S& s) { std::ranges::empty(s); });
41

From context, I suspect you meant to mark this function noexcept.

90

If this was cut-and-pasted from the ranges::size tests... should this now say RangeEmptyT?

Thanks for the review, Arthur. Will update shortly.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
90

This (and below) is intentional. I wanted to make sure that this wasn't choosing the __can_invoke_size overload.

zoecarver added inline comments.Apr 23 2021, 2:51 PM
libcxx/include/__ranges/empty.h
51–54

Good catch, thanks. I'm not going to do bool(X) for the overload on L57 because we know that it's a primitive type.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
25–30

Finally, please add a test that intentionally has a non-const size and empty:
struct S { int size(); bool empty(); };
static_assert(!requires (const S& s) { std::ranges::size(s); });
static_assert(!requires (const S& s) { std::ranges::empty(s); });

That test will fail to compile, but I get what you mean. Added.

zoecarver updated this revision to Diff 340172.Apr 23 2021, 2:52 PM
  • Apply Arthur's comments.
  • Rebase.

As usual, replace X with bool(X) in the noexcept-spec and also in the return. Also, __t should be an lvalue according to "let t be an lvalue that denotes the reified object for E." So:

This comment actually goes for all recent CPOs (begin, end, size, etc.).

CC @cjdb.

cjdb added a subscriber: tcanens.Apr 23 2021, 3:31 PM
cjdb added inline comments.
libcxx/include/__ranges/empty.h
51–54

and also in the return

This shouldn't be necessary thanks to boolean-testable. @tcanens I don't think the wording permit us to use it right now; did we miss one in the cleanup?

tcanens added inline comments.
libcxx/include/__ranges/empty.h
39

I'd expect this one and the next to require !__member_empty?

49

Doesn't clearly implement a case corresponding to the bullet point ["If T is an array of unknown bound, ranges​::​empty(E) is ill-formed."](https://eel.is/c++draft/range.access#range.prim.empty-2.1)
But I think that bullet point is actually redundant; arrays of unknown bound also don't have ranges::end(E) or ranges::size(E) or t.begin().

The difference is that for array of unknown bound of incomplete type, ranges::begin / ranges::end are IFNDR, but ranges::empty is ill-formed DR.

Do we understand the intent (in all these CPOs) when T is a reference to an array of unknown bound?

T is the type of some expression. Expressions don't have reference type.

51–54

and also in the return

This shouldn't be necessary thanks to boolean-testable. @tcanens I don't think the wording permit us to use it right now; did we miss one in the cleanup?

This one doesn't even require the result to be implicitly convertible to bool, which is probably why I didn't touch it with boolean-testable. I don't know what the original intended design is - @CaseyCarter?

ldionne added inline comments.Apr 29 2021, 2:16 PM
libcxx/include/__ranges/empty.h
39

I think the expression we check should be ranges::size(_VSTD::forward<_Tp>(__t)) == 0 instead, according to the spec.

I'm actually not sure why the spec mandates checking == 0 since ranges::size(E) is always supposed to be integer-like, but oh well.

71

Do we need const here? Shouldn't constexpr imply const for objects? I thought I didn't see it on the other CPOs.

zoecarver added inline comments.Apr 29 2021, 2:39 PM
libcxx/include/__ranges/empty.h
39

@tcanens you're right. @ldionne I don't think we need the == 0 because the return type is integral. But you might be right about forwarding the expression. I know __t is required to be an lvalue, so that's why I didn't have it originally. I guess it comes down to whether or not a copy is going to happen when we call size, and I don't think we can guarantee that won't happen, so I think maybe we should forward it.

  • Apply review comments: a. Remove const. b. Forward to ranges::size. c. Check no member.

Add:

// XFAIL: msvc && clang
ldionne accepted this revision.Apr 30 2021, 1:52 PM

LGTM with nitpick.

libcxx/include/__ranges/empty.h
3

Woops, still not done. Actually, I've been removing the file names from the start of the files. I don't think they add anything, they just get out of sync like this. I'd just remove it.

This revision is now accepted and ready to land.Apr 30 2021, 1:52 PM
cjdb accepted this revision.Apr 30 2021, 3:21 PM

LGTM with semiregular test.

cjdb added a comment.May 6 2021, 11:49 AM

Ping. Can this be merged or is there a blocker besides CI?

Quuxplusone added inline comments.May 6 2021, 1:38 PM
libcxx/include/__ranges/empty.h
49

@tcanens: "Expressions don't have reference type" — What if T is the type of the expression foo(), where foo is declared as int (&foo())[]? Wouldn't foo() have reference type then? Or would you simply say it has type "array of unknown bound" and is an lvalue?
Anyway, libstdc++ and MSVC both agree that in that case ranges::empty should SFINAE away, which is the expected/simple/obvious answer anyway.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
13

Remove XFAIL here (and throughout); the msvc issue is fixed.

26

Add:

static_assert(!std::is_invocable_v<RangeEmptyT, int(&)[]>);
static_assert(!std::is_invocable_v<RangeEmptyT, int(&&)[]>);
72

constexpr size_t size() const { return size_; }

97

I'm still confused about why sentinel needs to be comparable to every-iterator-type-ever.
I would rather see it scoped down to whatever specific type you're planning to compare it to (via nested member types, as shown in another of these reviews).

112–113

You have some pass-by-const-value here. Remove const. Also, remove the definition and constexprness from any of these functions that you don't expect to be called.

129

constexpr size_t size() const { return 1; }

tcanens added inline comments.May 6 2021, 3:31 PM
libcxx/include/__ranges/empty.h
49

Or would you simply say it has type "array of unknown bound" and is an lvalue?

This. See [expr.type]/1.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
26

I'd suggest a test case for "array of unknown bound of incomplete type" (ideally for all the ranges CPOs). We spent a decent chunk of time in LWG pinning down what should happen for those.

tcanens added inline comments.May 6 2021, 4:14 PM
libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
26

Yep. For empty and size it's simpler since those are supposed to be ill-formed DR.

cjdb commandeered this revision.May 7 2021, 4:47 PM
cjdb edited reviewers, added: zoecarver; removed: cjdb.

Commandeering with @zoecarver's permission.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp
97

The effort-to-reward ratio for this doesn't really pay off. What do we gain from doing this? What do we lose by keeping it as-is?

  • Apply review feedback and rebase.
  • Fix comment in empty.incomplete.verify.cpp
  • Fix bots: change expected-error regex.
This revision was automatically updated to reflect the committed changes.