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

Convert array bounds to IntLiteral. #4526

Merged

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Nov 14, 2024

Instead of leaving array bounds as whatever integer type they arrive as, convert them to the IntLiteral type as part of forming an ArrayType. This ensures that array types canonicalize properly even when the bounds are specified with different types.

Create an empty generic definition for a generic builtin function to avoid this causing "use of undefined generic function" errors.

@zygoloid
Copy link
Contributor Author

Depends on #4525. Review only the last commit here.

sized integer types.

In preparation for changing integer literals to be of `IntLiteral` type.
Conversions from the integer literal type are only permitted when the
value fits within the destination type.

For now, if conversion cannot be checked because the source value is a
symbolic constant, produce a symbolic constant representing the
conversion rather than rejecting it.
@zygoloid zygoloid force-pushed the toolchain-convert-array-bounds branch from 3964c95 to 3a61b81 Compare November 14, 2024 00:48
Don't hold a Function& after it may have been invalidated by importing.
@zygoloid zygoloid force-pushed the toolchain-convert-array-bounds branch from 3a61b81 to eb99749 Compare November 14, 2024 01:36
@zygoloid zygoloid added the dependent Depends on another issue/PR label Nov 14, 2024
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Should we be trying to reduce the noise in these imports? This seems to be 700 lines per test file using i32 (I'm guessing 10-20 KB?), repeated per-test for split files (e.g., see the delta for array.carbon). Also, presumably will increase churn when an interface is added. I think there's some benefit to the full representation, but this is big enough that I'm wondering if we should be more concerned about long-term repo size issues.

The basics of this PR seem okay, but what's the filesize of the 80K line delta?

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Approving but not merging. The changes here seem fine to me, and I think my one concern will be addressed through #4534.

@zygoloid zygoloid added this pull request to the merge queue Nov 15, 2024
Merged via the queue into carbon-language:trunk with commit 980ce6b Nov 15, 2024
8 checks passed
@zygoloid zygoloid deleted the toolchain-convert-array-bounds branch November 15, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent Depends on another issue/PR toolchain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants