-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix big integer literals type #6234
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
Changes from 2 commits
ccb331f
b084046
f119696
4ce25c1
3b60f22
670396e
a4c0a71
c43067e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,9 +28,12 @@ | |||||||
|
|
||||||||
| namespace Carbon::Check { | ||||||||
|
|
||||||||
| // Find the bit width of an integer literal. | ||||||||
| // The default bit width is 32. If the literal's bit width is greater than 32, | ||||||||
| // the bit width is increased to 64. | ||||||||
| // Find the bit width of an integer literal. Following the C++ standard rules | ||||||||
| // for assigning a type to a decimal integer literal, the first signed integer | ||||||||
| // in which the value could fit among bit widths of 32, 64 and 128 is selected. | ||||||||
| // If the value can't fit into a signed integer with width of 128-bits, then a | ||||||||
| // diagnostic is emitted and the maximum width of 128-bits is returned. Returns | ||||||||
| // IntId::None if the argument is not a constant integer or is symbolic. | ||||||||
| static auto FindIntLiteralBitWidth(Context& context, SemIR::InstId arg_id) | ||||||||
| -> IntId { | ||||||||
| auto arg_const_id = context.constant_values().Get(arg_id); | ||||||||
|
|
@@ -39,16 +42,29 @@ static auto FindIntLiteralBitWidth(Context& context, SemIR::InstId arg_id) | |||||||
| arg_const_id.is_symbolic()) { | ||||||||
| // TODO: Add tests for these cases. | ||||||||
| return IntId::None; | ||||||||
| ; | ||||||||
| } | ||||||||
| auto arg = context.insts().GetAs<SemIR::IntValue>( | ||||||||
| context.constant_values().GetInstId(arg_const_id)); | ||||||||
| unsigned arg_non_sign_bits = | ||||||||
| context.ints().Get(arg.int_id).getSignificantBits() - 1; | ||||||||
| llvm::APInt arg_val = context.ints().Get(arg.int_id); | ||||||||
| unsigned arg_non_sign_bits = arg_val.getSignificantBits() - 1; | ||||||||
|
|
||||||||
| // TODO: What if the literal is larger than 64 bits? Currently an error is | ||||||||
| // reported that the int value is too large for type `i64`. Maybe try to fit | ||||||||
| // in i128/i256? Try unsigned? | ||||||||
| return (arg_non_sign_bits <= 32) ? IntId::MakeRaw(32) : IntId::MakeRaw(64); | ||||||||
| // Value doesn't fit to any C++ supported signed integer, diagnose and return | ||||||||
| // the maximum supported integer bit width. | ||||||||
| if (arg_non_sign_bits >= 128) { | ||||||||
| CARBON_DIAGNOSTIC(IntTooLargeForCppType, Error, | ||||||||
| "integer value {0} too large to be fitted in any " | ||||||||
| "supported signed C++ type, max supported _int128", | ||||||||
|
||||||||
| "integer value {0} too large to be fitted in any " | |
| "supported signed C++ type, max supported _int128", | |
| "integer value {0} too large to fit in a signed C++ integer type; requires {1} bits, but max is 128", |
where {1} is arg_non_sign_bits + 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg, done.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we want to turn anything where we diagnosed an error into an ErrorInst, which we then use as a signal to avoid diagnosing more errors against the same piece of code.
There is no IntId::Error though, so I understand why you'd make this choice here. The result is that we might diagnose multiple errors against this same integer though.
It's also a little inconsistent with the choice for arg_const_id == SemIR::ErrorInst::ConstantId where if we saw an error we return None, but if we generate an error we return 128.
I think the latter looks the most odd to me. What do you think of returning None here when we make an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sg. As you say it's more consistent with the other error cases, and also I find it good that it doesn't continue with the type i128 to look for the best overload.
How do you find the follow-up message in the test fail_import_large_int_literal.carbon though - call argument of type ``Core.IntLiteral`` is not supported? Shall we add the bit width as well, or the previous message contains enough information about the size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want two errors for the same piece of code, and we use ErrorInst to signal that something was diagnosed normally. Maybe we need to turn a None into an ErrorInst later, or we need to return an extra bit (an optional maybe?) to signify the difference of returning an error vs returning None in other cases, if nothing was diagnosed (like when it's symbolic?).
For now, we can put a TODO on the second error in the test , saying that we want to get rid of it. And we can follow up to do that in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the google style guide wants use to use
inthere since the value is clearly in range.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks.