This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] modifies `_CmpUnspecifiedParam` ignore types outside its domain

Authored by cjdb on May 1 2021, 9:03 PM.



D85051's honeypot solution was a bit too aggressive swallowed up the
comparison types, which made comparing objects of different ordering
types ambiguous.

Depends on D101707.

Diff Detail

Event Timeline

cjdb requested review of this revision.May 1 2021, 9:03 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2021, 9:03 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb added a reviewer: rsmith.May 1 2021, 9:13 PM

rejected both nullptr_t ([cmp.categories]/p3 notes that "nullptr_t meets this requirement" at the end of the paragraph)

So? That note is saying that it is conforming for an implementation to use nullptr_t as _CmpUnspecifiedParam, not that users are allowed to compare against nullptr_t.

cjdb updated this revision to Diff 342273.May 2 2021, 1:57 PM
cjdb retitled this revision from [libcxx] expands `_CmpUnspecifiedParam` to accept `nullptr_t` and ignore types outside its domain to [libcxx] modifies `_CmpUnspecifiedParam` ignore types outside its domain.
cjdb edited the summary of this revision. (Show Details)

removes the erroneous expansion to accommodate nullptr_t.

cjdb updated this revision to Diff 344116.May 10 2021, 10:36 AM

rebases to activate CI

cjdb updated this revision to Diff 344295.May 10 2021, 11:42 PM

rebases to activate CI

Mordante added inline comments.

Can you split the test macro in 2 marcos, a PASS and a FAIL? Then it's clear which of the tests are expected to a be valid and which should fail.

cjdb updated this revision to Diff 344498.May 11 2021, 11:18 AM

Turns TEST_OP into TEST_FAIL and adds TEST_PASS (requested by @Mordante)

Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.

Minor clang-format-induced misformatting here.


Btw, it is bizarre to me that (throughout the existing code) we take auto& of all these values, instead of making copies with auto. Is there a subtle reason for the & to be here (throughout)? My only wild guess is that it's something broken about constexpr?

Mordante requested changes to this revision.May 11 2021, 12:11 PM
Mordante added inline comments.

I think I wasn't clear what I meant for the TEST_PASS. After your change the 0L is valid. This is the reason the number of errors reduced from 18 to 12. So after removing the two lines above the test still passes. This was the part I wanted to have in TEST_PASS.

Is allowing 0L really intended? Looking at D85051 it seems @rsmith's intention to prohibit 0L.

If it's intended please adjust the unit test, the patch description, and the unit tests to split the TEST_FAIL unit test in a fail and pass part..


Even if I didn't ask for this macro, I still like it better than the previous code :-)

This revision now requires changes to proceed.May 11 2021, 12:11 PM
cjdb updated this revision to Diff 344661.May 11 2021, 10:16 PM

ensures 0L fails

cjdb added inline comments.May 11 2021, 10:17 PM

Fixed, although I'm not too happy with the solution. Feels kinda hacky?

cjdb updated this revision to Diff 344871.May 12 2021, 10:44 AM

applies @Mordante's offline feedback to fix hack

Mordante accepted this revision.May 12 2021, 11:10 AM

LGTM, please make sure the build passes before committing.

This revision is now accepted and ready to land.May 12 2021, 11:10 AM
cjdb updated this revision to Diff 344953.May 12 2021, 1:58 PM

tries to get CI on AppleClang working one more