fixes #16246 #18800
fixes #16246 #18800
Conversation
| @@ -1144,6 +1145,13 @@ proc sameTypeAux(x, y: PType, c: var TSameTypeClosure): bool = | |||
| of tyEmpty, tyChar, tyBool, tyNil, tyPointer, tyString, tyCstring, | |||
| tyInt..tyUInt64, tyTyped, tyUntyped, tyVoid: | |||
| result = sameFlags(a, b) | |||
| if result and PickyCAliases in c.flags: | |||
| # additional requirement for the caching of generics for importc'ed types: | |||
| # the symbols must be identical too: | |||
There was a problem hiding this comment.
Given that I should learn: Why is checking the flags here equivalent to the symbols being identical? Is it just because we have checked for equality of the actual type etc. before in this proc?
Also it seems confusing that sameFlags is called before, but then we check more flags here.
In general what makes reading compiler code hard for me is that most procs have no short comment about their intent, e.g. this sameTypeAux could use a couple of lines saying what is needed for a type to be considered the same for instance.
There was a problem hiding this comment.
Given that I should learn: Why is checking the flags here equivalent to the symbols being identical? Is it just because we have checked for equality of the actual type etc. before in this proc?
Because we have established they are of the type kind which for tyInt etc means they are atomic types. Arguably we could also have checked for a.sym == b.sym instead but this would be worse for type aliases to cint, e.g. type myint = cint.
Also it seems confusing that sameFlags is called before, but then we check more flags here.
sameFlags looks at the type flags but importc is stored as a sym flag. These are different things.
this sameTypeAux could use a couple of lines saying what is needed for a type to be considered the same for instance.
Nim's type system has both structural and nominal type system aspects and type equality is supposed to follow from the spec.
There was a problem hiding this comment.
Thanks for the clarification!
|
Thanks a lot! I just tested the changes locally and indeed it fixes the issue here as well in a different (but similar) piece of code than the one originally shown in the issue. It also fixes the problem I reported for the C++ backend that happened without ARC interestingly enough. |
That is not unexpected as Orc only made the bug manifest in a novel way, the bug was there from the beginning, more or less. |
No description provided.