Introduce {Integer,Long}SignumIs{Positive,Negative} Refaster rules
#822
Conversation
|
Looks good. No mutations were possible for these changes. |
| @AfterTemplate | ||
| @AlsoNegation | ||
| boolean after(double v) { | ||
| return Math.signum(v) == 1; |
There was a problem hiding this comment.
Here and in the @AfterTemplate below flags S1244: Equality tests should not be made with floating point values, which is fair. Options:
- Omit the
Math.signumrules. - Suppress the warning, since the this will do the right thing, as per the documentation.
Option (2) does force others to suppress the warning too, though, so perhaps (1) is better?
There was a problem hiding this comment.
Oof, returning a double/float that is fixed to 1.0 / 0.0 / -1.0 is an interesting design indeed 😿 IMO generally most uses of double/float should be considered very carefully, but more to the point, I guess I'd lean towards the consistency and keep it included.
There was a problem hiding this comment.
Hmm, IIUC the equality check will work right? Because the floating point of 1 equality check is just fine. However, based on the Sonar rule I'd say we should not rewrite this one. It would maybe be better to flag this one as something that is not something one should do in code.
Additionally, for people outside of Picnic that have setup Sonar with most rules enabled will have to suppress / work around this one after our rewite, if I'm correct.
There was a problem hiding this comment.
K, I rebased and dropped the Math rules, at least for now. That way we can merge the PR and defer the rest (to potentially never do it).
| @AfterTemplate | ||
| @AlsoNegation | ||
| boolean after(double v) { | ||
| return Math.signum(v) == 1; |
There was a problem hiding this comment.
Oof, returning a double/float that is fixed to 1.0 / 0.0 / -1.0 is an interesting design indeed 😿 IMO generally most uses of double/float should be considered very carefully, but more to the point, I guess I'd lean towards the consistency and keep it included.
Stephan202
changed the title
{Integer,Long,Math}SignumIs{Positive,Negative} Refaster rules{Integer,Long}SignumIs{Positive,Negative} Refaster rules
Stephan202
force-pushed
the
sschroevers/more-signum-rules
branch
from
1de627a to
da94fae
Compare
| @AfterTemplate | ||
| @AlsoNegation | ||
| boolean after(double v) { | ||
| return Math.signum(v) == 1; |
There was a problem hiding this comment.
K, I rebased and dropped the Math rules, at least for now. That way we can merge the PR and defer the rest (to potentially never do it).
|
Looks good. No mutations were possible for these changes. |
|
Kudos, SonarCloud Quality Gate passed!
|
Suggested commit message:
These suggestions follow from the discussion in #812.