fix CppcheckParserV2; allow multiple NewIssueLocations #1436
Conversation
|
Dear All. Sorry for the mess with non-squashed commits. The change is ready for review. Thank you in advance! |
| return line; | ||
| } | ||
|
|
||
| public String getMsg() { |
| public class CxxReportLocation { | ||
| final String file; | ||
| final String line; | ||
| final String msg; |
| * Each issues in SonarQube might have multiple locations; Encapsulate its | ||
| * properties in this structure | ||
| */ | ||
| public class CxxReportLocation { |
There was a problem hiding this comment.
Unit test for CxxReportLocation missing
| @@ -267,70 +269,104 @@ public static String resolveFilename(final String baseDir, @Nullable final Strin | |||
| * @param ruleId | |||
| * @param msg | |||
| */ | |||
| public void saveUniqueViolation(SensorContext sensorContext, String ruleRepoKey, | |||
| @Nullable String file, @Nullable String line, String ruleId, String msg) { | |||
| public void saveUniqueViolation(SensorContext sensorContext, String ruleRepoKey, @Nullable String file, | |||
There was a problem hiding this comment.
Would remove this function
| * @param ruleId | ||
| * @param locations | ||
| */ | ||
| public void saveUniqueViolation(SensorContext sensorContext, String ruleRepoKey, String ruleId, |
There was a problem hiding this comment.
Would call always this function from sensors.
| } | ||
|
|
||
| if (isInputValid(id, msg)) { | ||
| sensor.saveUniqueViolation(context, CxxCppCheckRuleRepository.KEY, file, line, id, msg); | ||
| if (locations.isEmpty()) { | ||
| sensor.saveUniqueViolation(context, CxxCppCheckRuleRepository.KEY, file, line, id, msg); |
There was a problem hiding this comment.
Would try to handle this always with locations
|
@ivangalkin thanks for your proposal. My main point is that I would unify the interface and would use always the version with CxxReportLocation list only. |
|
Hello Günter and thank you for the reivew! I will apply your comment in the nearest future. Unfortunately I found out, that CppcheckParserV2.java needs a bit more enhancement: For the issues with multiple locations the main message is not-equal to the message/info of the first location. E.g. According to my implementation the message becomes "Null pointer dereference", which differs from the previous logic and is quite misleading for the reader. The main message must still be "Possible null pointer dereference: pLd". So there will be more commits. Regards, Ivan |
* create first-class citizen entities CxxReportIssue and CxxReportLocation (1:n)
(CxxReportIssue::equals() allows clear distinguishing if issue was processed already)
* CppcheckParserV2: better implementation for primary and secondary locations
* CxxReportSensor: increase issues-per-module counter only once per CxxReportIssue,
increase issues-per-file counter only once per affected file
TODO (?): tests for CxxReportIssue and CxxReportLocation
TODO (?): use the new function CxxReportSensor::saveUniqueViolation(SensorContext, CxxReportIssue)
|
I applied all review comments, but had to re-submit the change as a new pull request #1447 (sorry, I misconfigured my fork). |
This change is