This is an archive of the discontinued LLVM Phabricator instance.

[libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()` for `vector<bool>'s `operator<=>`
ClosedPublic

Authored by H-G-Hristov on May 9 2023, 2:45 AM.

Details

Summary

An issue with operator() was found during the implementation of https://reviews.llvm.org/D132268.

This patch aims to resolve the issues by updating the operator to use perfect forwarding.

The original motivation for three_way_comp_ref_type is given in: https://reviews.llvm.org/D131395

three_way_comp_ref_type's implementation is inspired by comp_ref_type, which has two overloads:

template <class _Tp, class _Up>
bool operator()(const _Tp& __x,  const _Up& __y);

template <class _Tp, class _Up>
bool operator()(_Tp& __x,  _Up& __y);

__debug_three_way_comp is missing the first overload and also declares the typealias`_three_way_comp_ref_type ` incorrectly.

Diff Detail

Event Timeline

H-G-Hristov created this revision.May 9 2023, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 2:45 AM
Herald added a subscriber: yaxunl. · View Herald Transcript
H-G-Hristov retitled this revision from [libc++][spaceship] Fixed `three_way_comp_ref_type`'s `operator()` to [libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()`.May 9 2023, 3:43 AM
H-G-Hristov edited the summary of this revision. (Show Details)
H-G-Hristov edited the summary of this revision. (Show Details)

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

Try to fix CI

  • Use "perfect forwarding"
H-G-Hristov published this revision for review.May 14 2023, 5:16 AM
H-G-Hristov added a subscriber: philnik.

@philnik Could you please have a look at this patch?

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.
libcxx/include/__algorithm/three_way_comp_ref_type.h
39

"three_way_comp_ref_type's implementation is inspired by comp_ref_type, which has two overloads:"

I'm a bit confused. Here you don't use two overloads. Can you explain the difference between the message and the implementation?

H-G-Hristov added inline comments.May 22 2023, 1:22 PM
libcxx/include/__algorithm/three_way_comp_ref_type.h
39

I replaced the two overloads in comp_ref_type with the perfect forwarding version as philnik recommended, which should has the same effect, right? Maybe I need to change the message?

H-G-Hristov added inline comments.May 22 2023, 1:40 PM
libcxx/include/__algorithm/three_way_comp_ref_type.h
39

Or would the implementation with the two overloads as comp_ref_type more appropriate? I am not sure entirely.

H-G-Hristov edited the summary of this revision. (Show Details)May 22 2023, 1:41 PM
This comment was removed by H-G-Hristov.
H-G-Hristov retitled this revision from [libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()` to [libc++][spaceship] Fixed `__debug_three_way_comp`'s `operator()` for `vector<bool>'s `operator<=>`.Jun 5 2023, 8:16 AM
H-G-Hristov edited the summary of this revision. (Show Details)
philnik accepted this revision.Jun 9 2023, 8:55 AM

LGTM % nit.

libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
158–164

It's at most that number of comparisons, so we shouldn't check for equality anyways.

This revision is now accepted and ready to land.Jun 9 2023, 8:55 AM
  • Addressed comments
H-G-Hristov marked 2 inline comments as done.Jun 9 2023, 9:45 PM

@philnik Thank you for the review!