Eliminate wrongly used static variables #1605
Merged
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
There is no need to share state between classes. This is error-prone and prohibits any possible parallelization of code analysis * BullseyeParser * HardcodedAccountCheck - refactored * HardcodedIpCheck - refactored
ivangalkin
commented
Nov 27, 2018
|
|
||
| if (!regEx.isEmpty()) { | ||
| if (!regularExpression.isEmpty()) { |
| Objects.requireNonNull(regularExpression, "getRegularExpression() should not return null"); | ||
|
|
||
| if (null != regEx && !regEx.isEmpty()) { | ||
| if (!regularExpression.isEmpty()) { |
There was a problem hiding this comment.
probably throwing an IllegalStateException in case of empty regular expression would be more correct
There was a problem hiding this comment.
@ivangalkin general rule we had in the past:
- configuration errors should result in errors and "stop" the analysis
- runtime errors: https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Supported-configuration-properties#sonarcxxerrorrecoveryenabled
Wrong regex is configuration error.
There was a problem hiding this comment.
Ok. Then I'll throw an exception if regular expression was empty
guwirth
approved these changes
Nov 28, 2018
| Objects.requireNonNull(regularExpression, "getRegularExpression() should not return null"); | ||
|
|
||
| if (null != regEx && !regEx.isEmpty()) { | ||
| if (!regularExpression.isEmpty()) { |
There was a problem hiding this comment.
@ivangalkin general rule we had in the past:
- configuration errors should result in errors and "stop" the analysis
- runtime errors: https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Supported-configuration-properties#sonarcxxerrorrecoveryenabled
Wrong regex is configuration error.
| @@ -39,9 +39,9 @@ | |||
| public class BullseyeParser extends CxxCoverageParser { | |||
|
|
|||
| private static final Logger LOG = Loggers.get(BullseyeParser.class); | |||
| private static String prevLine; | |||
There was a problem hiding this comment.
@ivangalkin don't know why they were static in the past? Seems ok now.
Closed
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.
There is no need to share state between classes. This is error-prone and
prohibits any possible parallelization of code analysis
This change is