-
Couldn't load subscription status.
- Fork 246
Chore: Code hygiene - warn-numeric-widen #2588
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Andy HF Kwok <[email protected]>
Signed-off-by: Andy HF Kwok <[email protected]>
Signed-off-by: Andy HF Kwok <[email protected]>
Signed-off-by: Andy HF Kwok <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2588 +/- ##
============================================
+ Coverage 56.12% 59.30% +3.18%
- Complexity 976 1444 +468
============================================
Files 119 146 +27
Lines 11743 13747 +2004
Branches 2251 2353 +102
============================================
+ Hits 6591 8153 +1562
- Misses 4012 4373 +361
- Partials 1140 1221 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| <arg>-feature</arg> | ||
| <arg>-Xlint:_</arg> | ||
| <arg>-Ywarn-dead-code</arg> | ||
| <!-- <arg>-deprecation</arg>--> |
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.
Are these intended changes down here?
| record.add(0, i % 2 == 0) | ||
| record.add(1, i.toByte) | ||
| record.add(2, i.toShort) | ||
| record.add(1, i.toByte.toInt) |
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.
its probably can be directly casted to int?
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.
I believe overflow behaviour will be different and lead to unexpected and incorrect results
| record.add(0, i % 2 == 0) | ||
| record.add(1, i.toByte) | ||
| record.add(2, i.toShort) | ||
| record.add(1, i.toByte.toInt) |
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.
same, can be probably converted directly
| case Some(i) => | ||
| record.add(0, i.toByte) | ||
| record.add(1, i.toShort) | ||
| record.add(0, i.toByte.toInt) |
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.
same
Which issue does this PR close?
This PR aims to address the
-Wnumeric-widenwarning in the codebase.As the overall scope of strict-warning compliance is quite large, this change focuses on resolving this specific warning type as a first step. Future PRs will continue addressing other warning incrementally.
Partially closes #. #2255
Rationale for this change
To clean up the code as per static check with the hope to resolve all warning and promote the restrict warning profile for the default CI build to uphold Scala code quality.
What changes are included in this PR?
To perform explicit type conversion instead of relying on runtime implicit cast.
How are these changes tested?
-Wnumeric-widenoption on.