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

Implicit conversions for aggregates #981

Merged
merged 9 commits into from
Jan 6, 2022
Merged

Implicit conversions for aggregates #981

merged 9 commits into from
Jan 6, 2022

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Dec 9, 2021

Define initialization, assignment, comparison, and implicit conversion between data classes with different field orders and/or types.

Implements the decision made in #710 .

@josh11b josh11b added the proposal A proposal label Dec 9, 2021
@josh11b josh11b requested a review from a team December 9, 2021 20:27
@josh11b josh11b marked this pull request as ready for review December 10, 2021 21:00
@josh11b josh11b requested a review from a team as a code owner December 10, 2021 21:00
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Dec 10, 2021
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Looks good to me.

For example, since
[comparison between `i32` and `u32` is defined](/proposals/p0702.md#built-in-comparisons-and-implicit-conversions),
equality comparison between values of types `{.x: i32, .y: i32}` and
`{.y: u32, .x: u32}` is as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should say what order the fields are compared in. IIRC we said we'd use the order of the left-hand operand so that the order of operations in == matches =. Eg, "Equality and inequality comparisons compare fields using the field order of the left-hand operand and stop once the outcome of the comparison is determined. However, the comparison order and short-circuiting are generally expected to affect only the performance characteristics of the comparison and not its meaning."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, added.

docs/design/classes.md Outdated Show resolved Hide resolved
proposals/p0981.md Outdated Show resolved Hide resolved

Rather than picking the left-hand argument's field order when the orders were
different, we considered requiring the field order to match when performing
comparisons. An explicit conversion to a common type would be required to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
comparisons. An explicit conversion to a common type would be required to
equality comparisons. An explicit conversion to a common type would be required to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a larger change.

@chandlerc
Copy link
Contributor

LGTM as well!!!

@josh11b josh11b merged commit 21229b7 into carbon-language:trunk Jan 6, 2022
@josh11b josh11b deleted the compare branch January 6, 2022 19:16
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jan 6, 2022
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Define initialization, assignment, comparison, and implicit conversion between data classes with different field orders and/or types.

Implements the decision made in #710 .

Co-authored-by: Richard Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants