Page MenuHomePhabricator

[libc++][ranges] Implement `permutable`.
ClosedPublic

Authored by var-const on Feb 8 2022, 1:21 AM.

Details

Reviewers
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Commits
rG8f1d8785df92: [libc++][ranges] Implement `permutable`.

Diff Detail

Event Timeline

var-const created this revision.Feb 8 2022, 1:21 AM
var-const requested review of this revision.Feb 8 2022, 1:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2022, 1:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const updated this revision to Diff 406728.Feb 8 2022, 1:22 AM

Fill in the link to the patch on the Ranges status page.

var-const added inline comments.Feb 8 2022, 1:24 AM
libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.subsumption.compile.pass.cpp
20

Please let me know if these subsumption tests aren't applicable.

Quuxplusone added a subscriber: Quuxplusone.

LGTM % nits!

libcxx/docs/Status/RangesPaper.csv
69

Could mark this "In Progress" now.

libcxx/include/__iterator/permutable.h
28–30

Perhaps 2-space indent?
Personally I'd use _Ip for the template parameter name, but whatever.

libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.compile.pass.cpp
39

Instead of customizing iter_swap for forward_iterator, I'd prefer to see a hidden-friend swap for NonCopyable. That would still satisfy all your requirements for this test, right?

46–49

I agree with your logic. Spelling it out here doesn't seem useful, though, because it's not the form of the argument that's difficult; it's whether all of the premises are (still) factually correct. The reader has to visit cppreference to find that out, which means they're not reading your comment after the first sentence anyway.

libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.subsumption.compile.pass.cpp
20

Works for me.
I would have done a single overload set, like

template<class I> void test_subsumption() requires std::forward_iterator<I>;
template<class I> void test_subsumption() requires std::indirectly_movable_storable<I, I>;
template<class I> void test_subsumption() requires std::indirectly_swappable<I, I>;
template<class I> constexpr bool test_subsumption() requires std::permutable<I> { return true; }
static_assert(test_subsumption<int*>()); // unambiguous

but I could believe that's too "clever." Whatever you want to do here is okay, AFAIC.

ldionne accepted this revision.Feb 8 2022, 10:27 AM
ldionne added a subscriber: ldionne.

LGTM with comments.

libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.compile.pass.cpp
46–49

I personally dislike "cryptic" or "clever" comments that state something without explaining it at all. It assumes that the reader will immediately understand why the statement is true.

This comment saves me time because I can read and understand the reasoning without having to work it out myself.

libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.subsumption.compile.pass.cpp
20

I think this is a shorter, more copy-pastable and generally simpler way to test for subsumption than what we seem to do elsewhere. I would encourage @var-const to use something like this.

I thought it out, and I *think* this is 100% equivalent to the tests we have right now. If I missed a difference, it might be worth pointing it out @Quuxplusone .

37

I would add a test for subsumption of forward_iterator, like you're doing for the other ones.

This revision is now accepted and ready to land.Feb 8 2022, 10:27 AM
var-const updated this revision to Diff 406939.Feb 8 2022, 12:42 PM
var-const marked 8 inline comments as done.

Address feedback

libcxx/include/__iterator/permutable.h
28–30

The indent width for split lines doesn't seem to be specified in the LLVM style guide, but clang-format thinks it's 4: https://github.com/llvm/llvm-project/blob/07486395d2d05c9c567994456774cafdcc1611d0/clang/lib/Format/Format.cpp#L1163
I think there is some convenience in having regular indentation differ from indentation for split lines. If you feel otherwise, let me know.

libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.compile.pass.cpp
39

Done. Yes, I think it's better this way.

46–49

I'd prefer to keep the comment (I'm open to shortening the explanation if you have any specific suggestions). I think it would make it easier for the reader to verify my logic if they don't have to make any assumptions, and should also make cross-checking with cppreference faster.

libcxx/test/std/iterators/iterator.requirements/alg.req.permutable/permutable.subsumption.compile.pass.cpp
20

Thanks! I like this a lot -- it's more concise and easier to copy and/or extend.

Quuxplusone added inline comments.Feb 9 2022, 2:29 PM
libcxx/include/__iterator/permutable.h
28–30

I think there is some convenience in having regular indentation differ from indentation for split lines

Huh, I'd think the exact opposite, because the notion of what counts as "one line, split," versus "two lines," is completely fluid in C++. I don't see any logic to having 4-space indents on

concept permutable =
    x;

but only 2-space indents on

concept otherable =
  requires (T x) {
    x;
  };

I see LLVM's default style sets the various indent widths to 2 or 4 in what I would characterize as an arbitrary fashion, and maybe it's even worth nailing them all to 2 in our .clang-format.

LLVMStyle.IndentWidth = 2;
LLVMStyle.ObjCBlockIndentWidth = 2;
LLVMStyle.ConstructorInitializerIndentWidth = 4;
LLVMStyle.ContinuationIndentWidth = 4;

So, yes I feel otherwise; but not a blocker.

var-const added inline comments.Feb 9 2022, 8:33 PM
libcxx/include/__iterator/permutable.h
28–30

I don't think these values are arbitrary -- the regular indent width is 2 (not sure why Objective-C blocks have a separate setting, but it makes sense for the code within blocks to be indented by 2 just like regular functions and lambdas) and the indent width for continuations is 4 (and they treat constructor initializer lists as continuations).

but only 2-space indents on

It should be

concept otherable =
    requires (T x) {
      x;
    };

Having requires on a separate line is a continuation; however, the contents of the requires clause should be formatted as usual, even though they are within a continuation.

The CI failure in modular build looks unrelated (the failure is

Failed Tests (1):
  llvm-libc++-shared.cfg.in :: libcxx/selftest/dsl/dsl.sh.py

)

This revision was landed with ongoing or failed builds.Feb 9 2022, 8:36 PM
This revision was automatically updated to reflect the committed changes.