Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[string.view], [string.view.comparison] Refactor string_view comparisons, exploiting rewritten candidates #6270

Closed

Conversation

dangelog
Copy link
Contributor

With the current semantics of rewritten candidates for the comparison operators, it is superfluous to specify both overloads

  operator==(basic_string_view, basic_string_view)
  operator==(basic_string_view, type_identity_t<basic_string_view>)

(ditto for operator<=>).

The second overload is necessary in order to implement the "sufficient additional overloads" part of [string.view.comparison], but it is also sufficient, as all the following cases

  • sv == sv
  • sv == convertible_to_sv
  • convertible_to_sv == sv

will, in fact, end up using it (directly, or after being rewritten with the arguments swapped).

The reason why we still do have both operators seems to be historical; there is an explanation offered here https://stackoverflow.com/a/70851101 .

Basically, there were 3 overloads before a bunch of papers regarding operator<=> and operator== were merged:

  1. op==(sv, sv) to deal with sv == sv;
  2. op==(sv, type_identity_t<sv>) and
  3. op==(type_identity_t<sv>, sv) to deal with sv == convertible and viceversa.

Overload n.1 was necessary because with only 2. and 3. a call like sv == sv would be ambiguous. With the adoption of the rewriting rules, overload n.3 has been dropped, without realizing that overload n.1 would then become redundant.

This commit makes the operators in [string.view.comparison] exposition-only, and changes the implementation example for the "sufficient additional overloads" so that it only uses the strategy employing type_identity_t.

@dangelog dangelog force-pushed the fix-stringview-equality-overloads branch from 94efcba to 96f5958 Compare June 10, 2023 10:09
…ons, exploiting rewritten candidates

With the current semantics of rewritten candidates for the comparison
operators, it is superfluous to specify both overloads

```
  operator==(basic_string_view, basic_string_view)
  operator==(basic_string_view, type_identity_t<basic_string_view>)
```

(ditto for `operator<=>`).

The second overload is necessary in order to implement the "sufficient
additional overloads" part of [string.view.comparison], but it is also
sufficient, as all the following cases

*  `sv == sv`
*  `sv == convertible_to_sv`
*  `convertible_to_sv == sv`

will, in fact, end up using it (directly, or after being rewritten with
the arguments swapped).

The reason why we still do have both operators seems to be historical;
there is an explanation offered here https://stackoverflow.com/a/70851101 .

Basically, there were _3_ overloads before a bunch of papers regarding
`operator<=>` and `operator==` were merged:

1. `op==(sv, sv)` to deal with `sv == sv`;
2. `op==(sv, type_identity_t<sv>)` and
3. `op==(type_identity_t<sv>, sv)` to deal with `sv == convertible` and
   viceversa.

Overload n.1 was necessary because with only 2. and 3. a call like
`sv == sv` would be ambiguous. With the adoption of the rewriting rules,
overload n.3 has been dropped, without realizing that overload n.1 would
then become redundant.

This commit makes the operators in [string.view.comparison]
exposition-only, and changes the implementation example for the
"sufficient additional overloads" so that it only uses the strategy
employing `type_identity_t`.

Note: I didn't find a way to add a \expos without overrunning the right
margin, so I renamed the section, like the exposition-only helpers in
[mdspan].
@dangelog dangelog force-pushed the fix-stringview-equality-overloads branch from 96f5958 to 5343d0d Compare June 10, 2023 10:19
@frederick-vs-ja
Copy link
Contributor

I think this PR is misusing the term "exposition only". Can we only remove the first overload in the example?

@dangelog
Copy link
Contributor Author

Hi,
Sure. I justed to be no confusion regarding the fact that op==(string_view, string_view) isn't supposed to be provided in that form at all. I'm not sure if "exposition only" is too "aggressive" (?)

@dangelog
Copy link
Contributor Author

Done the minimal change only in #6324.

@jwakely
Copy link
Member

jwakely commented Jun 21, 2023

With the current semantics of rewritten candidates for the comparison operators, it is superfluous to specify both overloads

Then the superfluous one should be removed, not marked as exposition-only, or left in the synopsis and removed from a note.

Either it's needed, or it's not. So either it should be there, or it shouldn't.

I think this should be an LWG issue, not an editorial change.

@jensmaurer jensmaurer added lwg Issue must be reviewed by LWG. not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking. labels Jun 21, 2023
@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Apr 1, 2024

The proper changes are contained in #6915 (applying LWG3950).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lwg Issue must be reviewed by LWG. not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants