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 comparis… LWG3950 #6324

Closed

Conversation

dangelog
Copy link
Contributor

…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

can in fact use 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 removes op==(sv,sv) from the note that suggests how to implement the "sufficient additional overloads". I've left the overload in the synopsis untouched, however, under the assumption that its presence there doesn't mean that an implementation must provide it.

…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`

can in fact use 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 removes `op==(sv,sv)` from the note that suggests how to
implement the "sufficient additional overloads". I've left the overload
in the synopsis untouched, however, under the assumption that its
presence there doesn't mean that an implementation *must* provide it.
@dangelog
Copy link
Contributor Author

Alternative to #6270.

@jwakely
Copy link
Member

jwakely commented Jun 21, 2023

This commit removes op==(sv,sv) from the note that suggests how to implement the "sufficient additional overloads". I've left the overload in the synopsis untouched, however, under the assumption that its presence there doesn't mean that an implementation must provide it.

I don't think that's a valid assumption ... although users are forbidden from taking the address of that function now, so I don't immediately see how they'd observe whether it's present. Either way, I dislike having declarations in a synopsis that are superfluous or just decorative.

@dangelog
Copy link
Contributor Author

This commit removes op==(sv,sv) from the note that suggests how to implement the "sufficient additional overloads". I've left the overload in the synopsis untouched, however, under the assumption that its presence there doesn't mean that an implementation must provide it.

I don't think that's a valid assumption ... although users are forbidden from taking the address of that function now, so I don't immediately see how they'd observe whether it's present. Either way, I dislike having declarations in a synopsis that are superfluous or just decorative.

I don't know if I'd call it "decorative", I somehow thought that I needed it as an "anchor" to define the semantics of operator==, even if then operator==(sv,sv) isn't actually provided. I can certainly open a LWG issue regarding this, but now I don't know any longer what resolution should I propose. Is there a precedent somewhere I can use as inspiration?

@jwakely
Copy link
Member

jwakely commented Jun 21, 2023

The semantics of equality comparison are defined by [string.view.comparison], not by non-existent declarations in the header synopsis.

If a single operator==(basic_string_view<charT, traits>, type_identity_t<basic_string_view<charT, traits>>) overload is sufficient to fully implement the required semantics, we can remove the prose and just specify precisely that.

So I would:

  • Replace the homogeneous declaration in the header synopsis with the one using type_identity_t.
  • Remove Example 1 from [string.view.comparisons] because it no longer adds anything of value.
  • Change the normative description of operator== to use type_identity_t.

Does the same argument apply for operator<=> as well? Can we remove all mention of "sufficient additional overloads" from the header synopsis and from [string.view.comparison] and just define a single overload? If that works, we can get rid of paragraph 1 and table [tab:string.view.comparison.overloads] entirely, and maybe just turn p1 into a note explaining what the use of type_identity_t does. The table seems redundant, since all those comparisons are automatically supported if we have just == and <=>.

@dangelog
Copy link
Contributor Author

The semantics of equality comparison are defined by [string.view.comparison], not by non-existent declarations in the header synopsis.

If a single operator==(basic_string_view<charT, traits>, type_identity_t<basic_string_view<charT, traits>>) overload is sufficient to fully implement the required semantics, we can remove the prose and just specify precisely that.

OK. I had the impression that the wording deliberately didn't specify operator== like this (i.e. using type_identity_t) in order to give implementations some leeway in how exactly they want to achieve the desidered results. In theory an implementation can also choose some other strategy that doesn't use type_identity_t at all but constraints, SFINAE, etc.

If all of this i just a someho silly pretense and there's no real reason not to specify it with type_identity_t (which, well, is precisely the tool for the job here), then let's go with it.

So I would:

* Replace the homogeneous declaration in the header synopsis with the one using `type_identity_t`.

* Remove _Example 1_ from [string.view.comparisons] because it no longer adds anything of value.

* Change the normative description of `operator==` to use `type_identity_t`.

Does the same argument apply for operator<=> as well?

It does.

Can we remove all mention of "sufficient additional overloads" from the header synopsis and from [string.view.comparison] and just define a single overload? If that works, we can get rid of paragraph 1 and table [tab:string.view.comparison.overloads] entirely, and maybe just turn p1 into a note explaining what the use of type_identity_t does. The table seems redundant, since all those comparisons are automatically supported if we have just == and <=>.

That's the idea. OK, I'll package something like this and send it over as a LWG defect, because now it sounds way bigger than editorial.

@jwakely
Copy link
Member

jwakely commented Jun 21, 2023

type_identity_t didn't exist in the standard when those overloads were added, so using it wasn't an option.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 10, 2023

@dangelog Could you have a go at @jwakely's suggestion?

@dangelog
Copy link
Contributor Author

Hello,

This is now https://cplusplus.github.io/LWG/issue3950 , I'm not sure if LWG likes the approach, the reflector thread was a bit inconclusive?

@jwakely
Copy link
Member

jwakely commented Nov 10, 2023

LWG might be able to look at it today.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 10, 2023

Ah OK, but if you have an LWG issue, then we don't need this one. Thanks!

@tkoeppe tkoeppe closed this Nov 10, 2023
@dangelog
Copy link
Contributor Author

I mean, feel free to cherry pick the commit at a later time, if it implements the desired resolution. :-)

A slightly different approach is in #6270 . I guess that can be closed as well for the same reason (not editorial)?

@jensmaurer jensmaurer changed the title [string.view], [string.view.comparison] Refactor string_view comparis… [string.view], [string.view.comparison] Refactor string_view comparis… LWG3950 Nov 10, 2023
@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 10, 2023

I mean, feel free to cherry pick the commit at a later time, if it implements the desired resolution. :-)

Thanks, that's a nice thought, though it's just a little bit impractical given that we approve issue resolutions as a single, large paper and then draft edits on the back of that. So the important information would be to inform whoever is drafting the edits at that point that this PR already exists. I think for larger changes that could definitely be a nice bit of help (and we need to make sure to make a note on, say, the motions issue), but in this simple case it's probably not going to matter much.

@Dani-Hub, @jwakely: Would it be possible to annoate the LWG issue with a note that wording exists already, so that we remember when the time comes to draft?

@jwakely
Copy link
Member

jwakely commented Nov 10, 2023

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants