This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] implement `std::ranges::set_symmetric_difference`
ClosedPublic

Authored by huixie90 on Jul 11 2022, 3:40 PM.

Details

Summary

[libc++][ranges] implement std::ranges::set_symmetric_difference

Diff Detail

Event Timeline

huixie90 created this revision.Jul 11 2022, 3:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2022, 3:40 PM
Herald added a subscriber: mgorny. · View Herald Transcript
huixie90 published this revision for review.Jul 12 2022, 3:21 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const retitled this revision from [libc++][ranges] implement `std::ranges::set_semmetric_difference` to [libc++][ranges] implement `std::ranges::set_symmetric_difference`.Jul 12 2022, 9:47 AM
var-const edited the summary of this revision. (Show Details)
var-const requested changes to this revision.Jul 12 2022, 11:17 AM
var-const added inline comments.
libcxx/include/__algorithm/ranges_set_symmetric_difference.h
49

Use ranges::less explicitly?

libcxx/include/__algorithm/set_symmetric_difference.h
15

Nit: seems unused?

39

Nit: this space looks weird -- is this done by clang-format?

75

Move?

86

Move?

libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.symmetric.difference/ranges_set_symmetric_difference.pass.cpp
82

Optional: do you think a short alias like template <class T> using R = UncheckedRange<T> would make these checks a little more readable?

387

Now that https://reviews.llvm.org/D129414 is merged, these tests (for member pointers) are no longer necessary.

445

Ditto -- these tests are no longer necessary.

480

Optional: testing both overloads adds a lot of boilerplate. Consider:
a) make numberOf* members of a struct, move the definitions of comp and proj1/proj2 out of the nested scopes, reset the struct (numberOf = {}) before each test case;
b) make the test a lambda taking a functor, invoke the lambda twice passing another lambda that invokes either overload as the functor.

543

Ditto -- these tests are no longer necessary.

This revision now requires changes to proceed.Jul 12 2022, 11:17 AM
philnik added inline comments.Jul 12 2022, 11:39 AM
libcxx/include/__algorithm/set_symmetric_difference.h
39

clang-format keeps the spacing as-is because of SpacesInAngles: Leave. I think it could be improved to remove whitespaces anwhere where there aren't two consecutive brackets, but you can just remove the whitespace and clang-format will be happy. This is so clang-format doesn't break C++03 code.

huixie90 updated this revision to Diff 444170.Jul 13 2022, 12:30 AM
huixie90 marked 10 inline comments as done.

addressed review feedback

huixie90 marked an inline comment as done.Jul 13 2022, 12:31 AM
huixie90 added inline comments.
libcxx/include/__algorithm/set_symmetric_difference.h
28–30

should these names be double underscored? if so, what is the naming convention?

afaik, __out is a nasty macro and __out_iter clashes with the constructor argument name.

philnik added inline comments.Jul 13 2022, 3:59 AM
libcxx/include/__algorithm/set_symmetric_difference.h
28–30

They should be called __in1_, __in2_ and __out_.

huixie90 updated this revision to Diff 444212.Jul 13 2022, 4:07 AM

rename variables

var-const accepted this revision.Jul 13 2022, 12:35 PM

Thanks for addressing the feedback!

This revision is now accepted and ready to land.Jul 13 2022, 12:35 PM
This revision was automatically updated to reflect the committed changes.
huixie90 marked an inline comment as done.