-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[java] Fix AvoidUsingOctalValues false-positive #5020
Conversation
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.
Thanks for the PR! We greatly appreciate the contribution.
I've added some comments to improve the code. We should also add a new test case for the AvoidUsingOctalValues
rule. You can do so adding a new <test-code>
node in AvoidUsingOctalValues.xml
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTNumericLiteral.java
Outdated
Show resolved
Hide resolved
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTNumericLiteral.java
Outdated
Show resolved
Hide resolved
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTNumericLiteral.java
Outdated
Show resolved
Hide resolved
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTNumericLiteral.java
Outdated
Show resolved
Hide resolved
I added some test cases and fixed the logic; the code now checks for a period in the literal along with a float or double suffix at the end (after the hex and binary check), since those do cause the base to change to base 10. |
Generated by 🚫 Danger |
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTNumericLiteral.java
Outdated
Show resolved
Hide resolved
Let's see what the regression tester shows, but it seems better. I'd probably just add a test case before merging on the case But other than that, if @pmd-test doesn't show anything weird, this is good to go. |
Added the float literal test case. Also added code to check for exponents (those can also change an octal to decimal) with a test case. |
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTNumericLiteral.java
Outdated
Show resolved
Hide resolved
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.
This looks great! Thanks!
Thank you @Gold856! |
Describe the PR
ASTNumericLiteral did not correctly determine the base when a number ended with a non-number (L, F, D, .) This PR checks if the number ends with one of those characters, and correctly returns a base of 10.
Related issues
Ready?
./mvnw clean verify
passes (checked automatically by github actions)