-
Notifications
You must be signed in to change notification settings - Fork 1.2k
KotlinSafeCallOperatorFilter should filter "unoptimized" safe call followed by elvis #1954
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
Conversation
67ee009 to
c5ab85e
Compare
c5ab85e to
26ff2fc
Compare
leveretka
left a comment
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.
Implementation seems to be correct, just a bit concerned about complexity of the if. Nothing too critical to not merge this PR fast as indeed it improves the Kotlin code coverage a lot!
|
|
||
| /** | ||
| * @return non pseudo-instruction preceding given | ||
| * @return non pseudo-instruction preceding given iff it has given opcode, |
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.
is iff necessary here? It seems to me just if + otherwise is good enough explanation.
| continue; | ||
| } | ||
| } | ||
| } else if (target.getType() != AbstractInsnNode.JUMP_INSN |
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.
Complexity of this if chain is too high. Maybe you can split the logic somehow? This will make the code more readable.
67530e1 to
17467da
Compare
This was overlooked in #1814
See #921 (comment)
For https://github.com/SonarSource/sonar-kotlin/tree/2.22.0.5972
prior to this change we find 102 safe call operator chains
and after 114
ie 12 more
For http://github.com/JetBrains/kotlin/tree/v2.0.0
prior to this change 5042
and after 5323
ie 281 more