-
Notifications
You must be signed in to change notification settings - Fork 26
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
disallow enum to enum conversion (require passing through ord
)
#294
Comments
"Accepted RFC" here means "let's try and see what it breaks". |
"let's try and see what it breaks" => "only" 2 packages, more precisely 2 LOC in total. |
Still needs a backwards compat switch but aside from that, ready to go. |
IMO, explicit conversions should be allowed. Implicit conversions not allowed. |
The RFC only targets explicit conversions as there are no implicit enum conversions in Nim. |
* fix nim-lang/RFCs#294 ; disallow enum <=> enum conversion * fix the runnableExamples that was the instigator of this RFC * legacy -d:nimLegacyConvEnumEnum * use -d:nimLegacyConvEnumEnum in important_package nimgame2 * add test for enum cast * improve changelog * add changelog: Changes affecting backward compatibility * cleanup changelog * fix changelog
May I know how this RFC was accepted without much reasoning? I'm worried about us introducing tiny breaking changes on every minor release. |
what makes you think it was done without much reasoning? when true:
type Koo = enum k1, k2
type Goo = enum g1, g2, g3
var a = g3
var b = a.Koo
echo b before nim-lang/Nim#16351: after nim-lang/Nim#16351:
nim c -d:nimLegacyConvEnumEnum main # Warning: enum to enum conversion is now deprecated this change (and (and see nim-lang/Nim#17928 which mentions -d:nimLegacyConvEnumEnum in the error message) |
Copying my reply from nim-lang/Nim#16351 (comment) : I'm not sure anymore wether this change is a good idea, the user is already explicit when writing |
the added syntax salt doesn't matter if this case occurs so rarely, and if it prevents illegal unintended conversions in the common case, eg where user intended conversion from (say) an int and instead got a silent conversion from another unrelated enum. Note that nim is strict about other conversions (apart from when true:
type A = object
a0: int
type B = object
b0: int
var a = A(a0: 1)
var b = a.B # error |
objects are the odd one out here, I think it's reasonable to argue that your snippet should actually work (if the objects are structurally equivalent, but it's not implemented yet). This isn't really about strictness IMO, the compiler was already strict, the conversion is explicit. |
Conversions between enums are unsafe and should be done via |
For distincts a conversion from When we say that the conversion should be done via |
Not in practice, no. In theory you can argue that there should be no connection between Your argument makes much more sense when you compare two different enum types to two different distinct types of the same base type, but well, then these distinct types have at least the same base type whereas an enum's base type is not |
Yeah, that's a better comparison. Even though an |
I disagree, too verbose
It might seem like that, but it isn't. No more error prone than myfloat.int |
But it is safer, I gave an example why that is, think about a generic |
But this isn't exclusive to when typeof(x) is enum:
T(ord(x))
else:
T(x) where |
This is not about raising, the conversion to a different enum is silent and can easily produce an invalid enum value! Type conversions in Nim cannot produce invalid bit patterns otherwise, in general. This implies that |
What do you mean by silent? It's explicit in the code, and a
Wouldn't it also implies that you need to cast from |
* fix nim-lang/RFCs#294 ; disallow enum <=> enum conversion * fix the runnableExamples that was the instigator of this RFC * legacy -d:nimLegacyConvEnumEnum * use -d:nimLegacyConvEnumEnum in important_package nimgame2 * add test for enum cast * improve changelog * add changelog: Changes affecting backward compatibility * cleanup changelog * fix changelog
proposal
rationale: this seems error prone and it's more explicit to pass through
ord
(and should result in same C code)The text was updated successfully, but these errors were encountered: