This is an archive of the discontinued LLVM Phabricator instance.

[libc++][regex] Uses operator<=> in sub_match.
ClosedPublic

Authored by Mordante on Aug 20 2022, 12:11 PM.

Details

Summary

The removal of operator!= in this header will be done in a separate
commit.

Note in the synopsis of P1614R2 there is a constexpr

template<class BiIter>
  constexpr auto operator<=>(const sub_match<BiIter>& lhs, const sub_match<BiIter>& rhs);

In the implementation of P1614R2 there isn't a constexpr

template<class BiIter>
  auto operator<=>(const sub_match<BiIter>& lhs, const sub_match<BiIter>& rhs);

There doesn't seem to be an LWG-issue, but it was fixed in the Standard
by removing the constexpr in b050fd474f11441942c88ef69b8622c8036656ac.

Implements part of:

  • P1614R2 The Mothership has Landed

Diff Detail

Event Timeline

Mordante created this revision.Aug 20 2022, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 12:11 PM
Mordante requested review of this revision.Aug 20 2022, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 12:11 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
avogelsgesang added a comment.EditedAug 20 2022, 1:01 PM

can you please add a new row to the status doc for the removal of the operator!= on match_results? So we don't forget about it...

libcxx/docs/Status/SpaceshipProjects.csv
80

please also add a link to this review

libcxx/include/regex
370
5058

given the class is named sub_match instead of submatch, I would prefer the name __sub_match_cat here

5058–5059

an empty line between the introduction of __submatch_cat and the operator<=> would make this a bit more readable

libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp
224–225

the parameters should align correctly below each other...

229–230
253

missing whitespace. Here and other places in this header

254–257

shouldn't this rather check sub_match?
Move below the typedef ... sub_match and test for sub_match?

269–270

replace by testComparisons(sm1, sm2, /*isEqual*/ x == y, /*isLess*/ x <= y)?

Same for the other remaining comparison tests

359

we should also add some test case for a character type which does not return a strong_ordering. Currently the static_cast<__submatch_cat<_BiIter>> is completely untested.

One way would be to use the same approach as in https://reviews.llvm.org/D132268 (search for "WeakComp"; "WeakComp" is introduced in https://reviews.llvm.org/D131395)

avogelsgesang added inline comments.Aug 20 2022, 1:09 PM
libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp
269–270

missing

#if TEST_STD_VER > 17
    AssertOrderReturn<std::strong_ordering, std::basic_string<CharT>, submatch>();
#else
    AssertCompareReturnBool<std::basic_string<CharT>, submatch>();
#endif

same for the other comparisons

Mordante planned changes to this revision.Aug 21 2022, 5:24 AM
Mordante marked 9 inline comments as done.

Thanks for the review! I'll put this on hold until D131395 lands, then I can address your final review comment.

can you please add a new row to the status doc for the removal of the operator!= on match_results? So we don't forget about it...

I already have a patch for the operator!= removal, changing that in this patch will lead to merge conflicts so I will not add it here. I will upload that patch after this one lands; just to avoid merge conflicts.

libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp
253

I see I rely too much on clang-format ;-)

254–257

Guess I copy pasted it, but it indeed should be sub_match.

I'll put this on hold until D131395 lands, then I can address your final review comment.

Well, that might still take a while... That review turned out to be a suprisingly deep rat hole and time sink.

If you want to move ahead with this patch independently, please feel free to just copy the {Weak,Partial}Order helpers. I have no problem rebasing on your change, in case this review here lands first.

Mordante marked 2 inline comments as done.Aug 23 2022, 8:51 AM

I'll put this on hold until D131395 lands, then I can address your final review comment.

Well, that might still take a while... That review turned out to be a suprisingly deep rat hole and time sink.

If you want to move ahead with this patch independently, please feel free to just copy the {Weak,Partial}Order helpers. I have no problem rebasing on your change, in case this review here lands first.

I'm not really in a hurry with this patch, it's not really blocking other work and we're not near to branching a new version of LLVM. So I don't mind waiting.

@Mordante with D131395 landed, this should now be unblocked :)

@Mordante with D131395 landed, this should now be unblocked :)

Thanks for the heads up, I'll add it to my TODO list.

Mordante marked 2 inline comments as done.Apr 15 2023, 4:08 AM
Mordante updated this revision to Diff 513883.Apr 15 2023, 4:08 AM

Rebased and addresses review comments.

Mordante updated this revision to Diff 513889.Apr 15 2023, 4:48 AM

CI fixes.

Mordante updated this revision to Diff 513893.Apr 15 2023, 5:34 AM

CI fixes.

Zingam added a subscriber: Zingam.Apr 15 2023, 11:53 PM

can you please add a new row to the status doc for the removal of the operator!= on match_results? So we don't forget about it...

Just a reminder: I am working on to update the status page to include these too.

can you please add a new row to the status doc for the removal of the operator!= on match_results? So we don't forget about it...

Just a reminder: I am working on to update the status page to include these too.

Thanks for the update. Note the I have a patch locally with the changes for regex, these are not uploaded yet.

ldionne accepted this revision.Apr 20 2023, 9:15 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp
299–300

Could you do this instead to reduce the boilerplate?

template <class CharT, class Ordering>
struct char_traits : constexpr_char_traits {
    using comparison_category = Ordering;
};
This revision is now accepted and ready to land.Apr 20 2023, 9:15 AM
Mordante marked an inline comment as done.Apr 20 2023, 12:15 PM

Thanks for the review!

libcxx/test/std/re/re.submatch/re.submatch.op/compare.pass.cpp
299–300

Thanks for the suggestion, this looks a lot better!

Mordante updated this revision to Diff 515428.Apr 20 2023, 12:16 PM
Mordante marked an inline comment as done.

Rebased and addresses review comment.

Mordante updated this revision to Diff 515751.Apr 21 2023, 8:28 AM

Trigger CI.

Mordante updated this revision to Diff 515759.Apr 21 2023, 8:59 AM

Rebased to trigger CI.

Mordante updated this revision to Diff 515812.Apr 21 2023, 10:01 AM

Trigger CI.

This revision was landed with ongoing or failed builds.Apr 22 2023, 3:40 AM
This revision was automatically updated to reflect the committed changes.