-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Adding support for i32, i8 and i64 to-char conversion
#6425
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
base: trunk
Are you sure you want to change the base?
Changes from 1 commit
74abc7d
c7c60a7
6efd96c
4423c5c
088d976
0f722b6
c17f6ad
7366715
0b49ba9
6da16ea
edefca7
6cdd115
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,3 +27,7 @@ impl CharLiteral() as ImplicitAs(Char) { | |||||||||||
| impl CharLiteral() as As(Char) { | ||||||||||||
| fn Convert[self: Self]() -> Char = "char.convert_checked"; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| impl Int(32) as As(Char) { | ||||||||||||
| fn Convert[self: Self]() -> Char = "int.convert_char"; | ||||||||||||
| } | ||||||||||||
|
||||||||||||
| fn Convert[self: Self]() -> Char = "int.convert_char"; | |
| } | |
| fn Convert[self: Self]() -> Char = "int.convert_char"; | |
| } | |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -867,14 +867,6 @@ static auto ResolveSpecificDeclForInst(EvalContext& eval_context, | |||||||||||||||
| for (const auto& interface : info.self_impls_constraints) { | ||||||||||||||||
| ResolveSpecificDeclForSpecificId(eval_context, interface.specific_id); | ||||||||||||||||
| } | ||||||||||||||||
| for (const auto& constraint : info.extend_named_constraints) { | ||||||||||||||||
| ResolveSpecificDeclForSpecificId(eval_context, | ||||||||||||||||
| constraint.specific_id); | ||||||||||||||||
| } | ||||||||||||||||
| for (const auto& constraint : info.self_impls_named_constraints) { | ||||||||||||||||
| ResolveSpecificDeclForSpecificId(eval_context, | ||||||||||||||||
| constraint.specific_id); | ||||||||||||||||
| } | ||||||||||||||||
danakj marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
| break; | ||||||||||||||||
| } | ||||||||||||||||
| case CARBON_KIND(SemIR::SpecificId specific_id): { | ||||||||||||||||
|
|
@@ -991,6 +983,7 @@ static auto PerformCheckedCharConvert(Context& context, SemIR::LocId loc_id, | |||||||||||||||
| SemIR::InstId arg_id, | ||||||||||||||||
| SemIR::TypeId dest_type_id) | ||||||||||||||||
| -> SemIR::ConstantId { | ||||||||||||||||
|
|
||||||||||||||||
viniciusfdasilva marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||
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.
This looks like a copy paste that needs to be cleaned up?
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.
Yes!! I will remove this
viniciusfdasilva marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
danakj marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
[diff] reported by reviewdog 🐶
| SemIR::InstId arg_id, | |
| SemIR::TypeId dest_type_id) | |
| SemIR::InstId arg_id, SemIR::TypeId dest_type_id) |
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.
[diff] reported by reviewdog 🐶
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.
[diff] reported by reviewdog 🐶
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.
[diff] reported by reviewdog 🐶
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.
This surprised me, could you point me to where the design says this should happen, rather than converting anything up to 255?
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.
Actually, it’s a restriction I initially included, but it may indeed not make sense to keep it.
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.
Yes! I removed this code snippet! I don’t think it makes sense to add this check.
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.
[diff] reported by reviewdog 🐶
| CARBON_DIAGNOSTIC( | |
| NegativeIntInUnsignedType, Error, | |
| "character value {0} too large for type {1}", TypedInt, | |
| SemIR::TypeId); | |
| CARBON_DIAGNOSTIC(NegativeIntInUnsignedType, Error, | |
| "character value {0} too large for type {1}", TypedInt, | |
| SemIR::TypeId); |
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.
[diff] reported by reviewdog 🐶
| if (phase != Phase::Concrete) { | |
| if (phase != Phase::Concrete) { |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -373,6 +373,7 @@ constexpr BuiltinInfo PrintInt = { | |||||||||
| constexpr BuiltinInfo ReadChar = {"read.char", | ||||||||||
| ValidateSignature<auto()->AnySizedInt>}; | ||||||||||
|
|
||||||||||
|
|
||||||||||
danakj marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||
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.
[diff] reported by reviewdog 🐶
| constexpr BuiltinInfo IntConvertChar = {"int.convert_char", | |
| ValidateSignature<auto(AnyInt)->CharCompatible>}; | |
| constexpr BuiltinInfo IntConvertChar = { | |
| "int.convert_char", ValidateSignature<auto(AnyInt)->CharCompatible>}; |
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.
Can we make this generic for all integer sizes, like it is for
u8?carbon-lang/core/prelude/types/uint.carbon
Line 45 in 7be6538
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.
Good idea!
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.
Hi @danakj
I had issues with this kind of implementation, but I managed to get support working for i32, i8, and i64! However, I’ll work on adapting it to what you suggested in the future.
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.
Could you say more what issues you had? I think we should be able to write this as a single generic.