allow conversions between var types of range types and base types
#24037
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Maybe |
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Sep 3, 2024
Araq
pushed a commit
that referenced
this pull request
Sep 6, 2024
refs #21682, refs #24038 The `rangeBase` typetrait added in #21682 which gives the base type of a range type is now added publicly to `typetraits`. Previously it was only privately used in `repr_v2` and in `enumutils` since #24052 (coincidentally I didn't see this until now). This is part of an effort to make range types easier to work with in generics, as mentioned in #24038. Its use combined with #24037 is also tested. The condition for the "enum to enum conversion" warning is now also restricted to conversions between different enum base types, i.e. conversion between an enum type and a range type of itself doesn't give a warning. I put this in this PR since the test gave the warning and so works as a regression test.
Araq
pushed a commit
that referenced
this pull request
Sep 17, 2024
fixes #24097 For `nkConv` addresses where the conversion is between 2 types that are equal between backends, treat assignments the same as assignments to the argument of the conversion. In the VM this seems to be in `genAsgn` and `genAsgnPatch`, as evidenced by the special logic for `nkDerefExpr` etc. This doesn't handle ranges after #24037 because `sameBackendType` is used and not `sameBackendTypeIgnoreRange`. This is so this is backportable without #24037 and another PR can be opened that implements it for ranges and adds tests as well. We can also merge `sameBackendTypeIgnoreRange` with `sameBackendType` since it doesn't seem like anything that uses it would be affected (only cycle checks and the VM), but then we still have to add tests.
Araq
pushed a commit
that referenced
this pull request
Sep 29, 2024
fixes #22523 There were 2 problems with the code in `sameType` for `dcEqIgnoreDistinct`: 1. The code that skipped `{tyDistinct, tyGenericInst}` only ran if the given types had different kinds. This is fixed by always performing this skip. 2. The code block below that checks if `tyGenericInst`s have different values still ran for `dcEqIgnoreDistinct` since it checks if the given types are generic insts, not the skipped types (and also only the 1st given type). This is fixed by only invoking this block for `dcEq`; `dcEqOrDistinctOf` (which is unused) also skips the first given type. Arguably there is another issue here that `skipGenericAlias` only ever skips 1 type. These combined fix the issue (`T` is `GenericInst(V, 1, distinct int)` and `D[0]` is `GenericInst(D, 0, distinct int)`). #24037 shouldn't be a dependency but the diff follows it.
narimiran
pushed a commit
that referenced
this pull request
Dec 20, 2024
fixes #24097 For `nkConv` addresses where the conversion is between 2 types that are equal between backends, treat assignments the same as assignments to the argument of the conversion. In the VM this seems to be in `genAsgn` and `genAsgnPatch`, as evidenced by the special logic for `nkDerefExpr` etc. This doesn't handle ranges after #24037 because `sameBackendType` is used and not `sameBackendTypeIgnoreRange`. This is so this is backportable without #24037 and another PR can be opened that implements it for ranges and adds tests as well. We can also merge `sameBackendTypeIgnoreRange` with `sameBackendType` since it doesn't seem like anything that uses it would be affected (only cycle checks and the VM), but then we still have to add tests. (cherry picked from commit 1fbb67f)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs #24032, split from #24036
Conversion from variables of range types or base types of range types to the other are now considered mutable for
varparams, similar to how distinct types are mutable when converted to their base type or vice versa. There are 2 main differences:nkChckRange, which is not generated for things like tuple/object fields.set[range[3..5]]vsset[range[0..5]].Since the convertibility check for
varparams and a check whether to emit a no-op fornkConv(and now alsonkChckRange) so that the output is still addressable both usesameType, we accomplish this by adding a new flag tosameTypethat ignores range types, but only when they're not nested in other types. The implementation for this might be flawed, I didn't include children of some metatypes as "nested in other types", but stuff liketyGenericInstparams are respected.