This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges][NFC] Implement the repetitive parts of the remaining range algorithms:
ClosedPublic

Authored by var-const on Jul 12 2022, 2:02 AM.

Details

Summary
  • create the headers (but not include them from <algorithm>);
  • define the niebloid and its member functions with the right signatures (as no-ops);
  • make sure all the right headers are included that are required by each algorithm's signature;
  • update CMakeLists.txt and the module map;
  • create the test files with the appropriate synopses.

The synopsis in <algorithm> is deliberately not updated because that
could be taken as a readiness signal. The new headers aren't included
from <algorithm> for the same reason.

Diff Detail

Event Timeline

var-const created this revision.Jul 12 2022, 2:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 2:02 AM
Herald added a subscriber: mgorny. · View Herald Transcript
var-const requested review of this revision.Jul 12 2022, 2:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 2:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

This should both make implementing the remaining algorithms easier by taking care of the most boring/repetitive parts, and make reviewing them easier by reducing the delta (and leaving only the meaningful changes in the delta).

While I deliberately omitted any algorithms currently under review, this will cause merge conflicts for any unuploaded work. However, those should be straightforward to resolve by just preferring local over upstream.

Could you remove the {prev, next}_permutation and rotate changes? I have already written these parts and this would just add unnecessary merge conflicts.

var-const updated this revision to Diff 443875.Jul 12 2022, 2:12 AM

Remove already-started algorithms, rebase on main.

Could you remove the {prev, next}_permutation and rotate changes? I have already written these parts and this would just add unnecessary merge conflicts.

Done.

Thanks for doing this. It is really useful.
Could you please remove the set_symmetric_difference bit as I have something in progress here
https://reviews.llvm.org/D129520

var-const updated this revision to Diff 443877.Jul 12 2022, 2:16 AM

Remove an in-progress algorithm.

Thanks for doing this. It is really useful.
Could you please remove the set_symmetric_difference bit as I have something in progress here
https://reviews.llvm.org/D129520

Done.

philnik accepted this revision.Jul 12 2022, 2:24 AM

LGTM. (I haven't checked all the signatures). Maybe the signatures aren't actually that useful, since @huixie90 and I use clang-format in the ranges algorithms AFAIK. Regardless, I think this is a good idea. Please wait for @huixie90 to also approve.

This revision is now accepted and ready to land.Jul 12 2022, 2:24 AM
huixie90 accepted this revision.Jul 12 2022, 2:28 AM

LGTM. please confirm if ranges_set_union test is in the right directory

libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_union.pass.cpp
2

is this file in the right directory?

var-const updated this revision to Diff 443887.Jul 12 2022, 2:47 AM

Fix test file in a wrong directory.

var-const marked an inline comment as done.Jul 12 2022, 2:47 AM
var-const added inline comments.
libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_union.pass.cpp
2

Thanks for spotting this!

var-const updated this revision to Diff 443888.Jul 12 2022, 2:48 AM
var-const marked an inline comment as done.

Fix wrong file path.

This revision was landed with ongoing or failed builds.Jul 12 2022, 2:49 AM
This revision was automatically updated to reflect the committed changes.
Mordante added inline comments.
libcxx/include/module.modulemap.in
321

In about two week we'll branch the LLVM 15 release. Should we remove the not implemented changes in the branch after branching?
I mainly wonder about the changes to this file and CMakeLists.txt. Without these the files with the broken placeholder algorithms aren't visible for users.

var-const added inline comments.Jul 13 2022, 12:37 PM
libcxx/include/module.modulemap.in
321

The placeholder headers aren't included in <algorithm>, so they shouldn't be visible through normal C++ means -- or do you mean they would be somehow visible in the build system?