Skip to content

Conversation

@AnjuDel
Copy link
Member

@AnjuDel AnjuDel commented Feb 9, 2021

No description provided.

@AnjuDel AnjuDel force-pushed the spaceship_string branch 2 times, most recently from 3f93268 to 16b4f25 Compare February 9, 2021 17:25
@AnjuDel AnjuDel force-pushed the spaceship_string branch 2 times, most recently from 9f95581 to a9d8f3e Compare February 9, 2021 18:43
@CaseyCarter CaseyCarter added cxx20 C++20 feature spaceship C++20 operator <=> labels Feb 9, 2021
@AnjuDel AnjuDel marked this pull request as ready for review February 12, 2021 19:05
@AnjuDel AnjuDel requested a review from a team as a code owner February 12, 2021 19:05
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to the team!

I could only find a readability nit

@AnjuDel AnjuDel force-pushed the spaceship_string branch 2 times, most recently from 7fc539d to c73cbdc Compare February 12, 2021 22:32
@AnjuDel
Copy link
Member Author

AnjuDel commented Feb 12, 2021

Welcome to the team!

Thanks!

@StephanTLavavej StephanTLavavej changed the title feature/spaceship: Clause Strings feature/spaceship: Clause 21: Strings Feb 13, 2021
@CaseyCarter CaseyCarter self-assigned this Feb 16, 2021
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment change, which I'll go ahead and apply.

Clarify `#else` comment.
@CaseyCarter CaseyCarter removed their assignment Feb 16, 2021
@StephanTLavavej
Copy link
Member

I've pushed a batch of changes:

  • Remove outdated test comment (unrelated to other changes).
    • This test has grown far beyond testing only spaceship for containers.
  • Fix/test char_traits::comparison_category.
    • We need to modify _WChar_traits and _Narrow_char_traits, because they inherit from private _Char_traits.
  • C++20 needs only one _Identity_t overload.
    • This was indicated by an edited example in the paper.
    • I've guarded the extra overload for operator== with #if !_HAS_CXX20, and simply dropped the extra overload for operator<=>.
    • The choice is arbitrary, but to match the Standard's example, I'm keeping the overload that takes _Identity_t on the RHS.
  • Spaceship should take basic_string_view by value, like other operators.
    • Also matches the Standardese.
  • Simplify syntax with an alias template.
    • It's repeated 8 times, so _Get_comparison_category_t significantly reduces verbosity.
    • The helper now emits type (to match _t) instead of comparison_category.
  • Implement LWG-3432 "Missing requirement for comparison_category".
    • There's a "Mandates" that we need to enforce with _Is_any_of_v.
  • Add SAL and top-level const for consistency.
    • I am still not a fan of SAL, but _In_z_ is probably the most useful, and we use it consistently in string.
  • Avoid negated _HAS_CXX20 condition.
    • We conventionally prefer to avoid negated conditionals (preprocessor, if, etc.) when doing so doesn't complicate the code. This makes the code easier for our human brains to understand.
    • As a bonus, this makes it easier to see the symmetry between the == and <=> used for C++20.
  • Test string <=> ptr and ptr <=> string.
    • string <=> ptr is implemented by a separate overload, so we need to test that. We expect the compiler to handle rewriting, but string is sufficiently complicated and important that spending 3 lines to test ptr <=> string is reasonable.
  • Test string_view at runtime and compiletime.
    • This could be refactored in the future for less verbosity (using our dual-mode runtime/compiletime pattern), but let's go with this for now.

@StephanTLavavej StephanTLavavej merged commit bbedd7a into microsoft:feature/spaceship Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature spaceship C++20 operator <=>

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants