Replace IsSubType check with IsSameTypeKind check where necessary
#1032
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
SupunS
changed the title
IsSubType check with IsSameTypeKind check where necessary
Codecov Report
@@ Coverage Diff @@
## master #1032 +/- ##
=======================================
Coverage 77.28% 77.29%
=======================================
Files 274 274
Lines 35414 35401 -13
=======================================
- Hits 27371 27362 -9
+ Misses 6958 6955 -3
+ Partials 1085 1084 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
SupunS
force-pushed
the
supun/never-type-checking
branch
from
1f9a426 to
1709c9f
Compare
SupunS
force-pushed
the
supun/never-type-checking
branch
2 times, most recently
from
65c2749 to
c090eef
Compare
turbolent
reviewed
Nov 23, 2021
SupunS
force-pushed
the
supun/never-type-checking
branch
from
c090eef to
0de4ee2
Compare
SupunS
force-pushed
the
supun/never-type-checking
branch
from
0de4ee2 to
e6be268
Compare
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 424f814 Results
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I searched all the usages and replaced them where necessary. Unfortunately, this has to be done manually, can't think of a way to automate this/ audit with a test case. The general rule is:
I had already added some comments to the newly added mehtod. Also added a note on the old |
|
@SupunS Nice! Could we keep this comment/guideline somewhere in the codebase? Maybe as a comment somewhere, or in |
turbolent
reviewed
Nov 24, 2021
Comment on lines
+4711
to
+4721
| // However, to check if a type *strictly* belongs to a certain category, then consider | ||
| // using `IsSameTypeKind` method. e.g: "Is type `T` an Integer type?". Using this method | ||
| // for the later use-case may produce incorrect results. | ||
| // * IsSubType() - To check the assignability. e.g: Is argument type T is a sub-type | ||
| // of parameter type R. This is the more frequent use-case. | ||
| // * IsSameTypeKind() - To check if a type strictly belongs to a certain category. e.g: Is the | ||
| // expression type T is any of the integer types, but nothing else. | ||
| // Another way to check is, asking the question of "if the subType is Never, | ||
| // should the check still pass?". A common code-smell for potential incorrect | ||
| // usage is, using IsSubType() method with a constant/pre-defined superType. | ||
| // e.g: IsSubType(<<someType>>, FixedPointType) |
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.
Closes #1031
Description
Neveras 'not' belongs to the same kind.masterbranchFiles changedin the Github PR explorer