handle duplicate issues with different relative paths

[guwirth]

Some sensors are creating same issues with different relative paths. These issues were added several times to source files in the past.

This PR normalize paths before adding them to avoid this.

Sample:

• xxx/aaa/bbb/../ddd/eee/source.cpp
• xxx/aaa/ccc/../ddd/eee/source.cpp
• => xxx/aaa/ddd/eee/source.cpp

close #1777

---

This change is 

[ivangalkin]

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guwirth and @jmecosta)

---

cxx-squid/src/main/java/org/sonar/cxx/utils/CxxReportLocation.java, line 37 at r1 (raw file):

  public CxxReportLocation(@Nullable String file, @Nullable String line, String info) {
    super();
    if (file != null) {

constructor of the class CxxReportLocation is a wrong place to implement any logic/transformation IMHO. It was designed as an analogue of NewIssueLocation (http://javadocs.sonarsource.org/6.0/apidocs/org/sonar/api/batch/sensor/issue/NewIssueLocation.html) and intends to capture the given arguments as is.

previously all path transformation and notmalizations were done in CxxIssuesReportSensor.java. Please have a look at #1653. I believe it's better to move the logic to this file, because it handles the resolution w.r.t. module's base directory.

Besides that the PR looks good to me. Path::nomalize()is ok. Just FYI: it's important NOT to use the canonical path because of #1575

[guwirth]

Hi @ivangalkin,

Thanks for your feedback.

The reason why I did it in CxxReportLocation is saveUniqueViolation. Here we check with a HashSet for duplicates of CxxReportIssue. This does not work with not normalized paths (and was in the past the reason for the duplicates). Think better would be to compare later in saveViolation after path is absolute. But this would be much slower?

Could also normalize inside of saveUniqueViolation?

Regards,

[Bertk]

LGTM

[ivangalkin]

@guwirth your explanation makes sense completely; caching of CxxReportIssue with non-normalized paths is wrong.

saveUniqueViolation

would be a better place IMHO since we don't scatter the path transforming logic across different
location. However I see that it requires rather bigger change.

So LGTM