Fix big integer literals type #6234
Conversation
toolchain/check/cpp/type_mapping.cpp
Outdated
| 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; |
There was a problem hiding this comment.
| unsigned arg_non_sign_bits = arg_val.getSignificantBits() - 1; | |
| int arg_non_sign_bits = arg_val.getSignificantBits() - 1; |
nit: I think the google style guide wants use to use int here since the value is clearly in range.
There was a problem hiding this comment.
Done! Thanks.
toolchain/check/cpp/type_mapping.cpp
Outdated
| "integer value {0} too large to be fitted in any " | ||
| "supported signed C++ type, max supported _int128", |
There was a problem hiding this comment.
How about something like this:
| "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
toolchain/check/cpp/type_mapping.cpp
Outdated
| TypedInt); | ||
| context.emitter().Emit(arg_id, IntTooLargeForCppType, | ||
| {.type = arg.type_id, .value = arg_val}); | ||
| return IntId::MakeRaw(128); |
There was a problem hiding this comment.
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.
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.
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.
Done. Thanks!
2 participants
Following the C++ standard rules for assigning a type to a decimal integer literal, when a Carbon integer literal is passed as a call argument to a C++ function and it is too big to fit to
int,longorlong long, it is assigned an extended integer type (_int128). Discussed in the interop meeting and documented in this design doc.Part of #5915