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

Provide generic impls for Core.Int and Core.UInt operations. #4693

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Dec 17, 2024

Instead of providing operations only for i32, provide them for all iN and uN types.

For now, this excludes the *Assign, Inc and Dec interfaces, because the implementations for those are defined as Carbon functions rather than builtins, and we can't yet lower definitions for specific functions, so converting those to be generic breaks the build for our examples.

@github-actions github-actions bot added documentation An issue or proposed change to our documentation toolchain labels Dec 17, 2024
@zygoloid zygoloid force-pushed the toolchain-generic-int-ops branch from b0dfa7f to d25e2d7 Compare December 17, 2024 00:46
We don't support lowering of specific function definitions yet, so avoid
defining generic functions in the prelude as that breaks the build for
all of our examples.
@zygoloid zygoloid marked this pull request as ready for review December 19, 2024 20:28
@github-actions github-actions bot requested a review from geoffromer December 19, 2024 20:28
Comment on lines 38 to 45
impl u32 as AddAssign {
fn Op[addr self: Self*](other: Self) {
*self = *self + other;
}
}
// TODO: Once we can lower specific functions, change this to:
// impl forall [N:! IntLiteral()] UInt(N) as Inc {
impl u32 as Inc {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think some of these spacing choices don't make as much sense to me... Like I get the choice to avoid whitespace between BitAnd and BitAndAssign, but why isn't Inc just on its own, and similar for Dec? I don't really view these as associated with Add/Sub -- sort of how Sub could be written in terms of Add and Negate (or Negate in terms of Sub), but are treated as distinct.

Copy link
Contributor Author

@zygoloid zygoloid Dec 19, 2024

Choose a reason for hiding this comment

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

This is pre-existing (copied from int.carbon), but IIRC the idea here was that they are grouped together because Inc is implemented in terms of AddAssign, so must be ordered after AddAssign -- the groups with blank lines between them can be put in any order (and are ordered alphabetically by the first interface name in the group) but should be kept as a group if they are reordered due to dependencies.

Perhaps this was more obvious when the Inc impl actually named the preceding AddAssign impl rather than implicitly using it via an operator.

Copy link
Contributor Author

@zygoloid zygoloid Dec 19, 2024

Choose a reason for hiding this comment

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

As an alternative, we could order these as:

  • basic operators, alphabetically
  • *Assign operators, alphabetically
  • Inc/Dec operators, alphabetically

Maybe that'd be nicer?

@zygoloid zygoloid enabled auto-merge December 19, 2024 21:43
@zygoloid zygoloid added this pull request to the merge queue Dec 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Dec 19, 2024
@zygoloid zygoloid enabled auto-merge December 19, 2024 21:57
@zygoloid zygoloid added this pull request to the merge queue Dec 19, 2024
Merged via the queue into carbon-language:trunk with commit 6fe8e0b Dec 19, 2024
8 checks passed
@zygoloid zygoloid deleted the toolchain-generic-int-ops branch December 19, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An issue or proposed change to our documentation toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants