use real path in coverage sensors also

[jmecosta]

This change is 

[ivangalkin]

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Bertk, @guwirth, and @jmecosta)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/utils/CxxUtils.java, line 99 at r1 (raw file):

  }

  public static InputFile getInputFileIfInProject(SensorContext sensorContext, String path, Set<String> notFoundFiles) {

TBH I don't really like this solution. CxxUtils is not a good place for such implementation. I would suggest to move

• private final Set notFoundFiles = new HashSet<>();
• public InputFile getInputFileIfInProject(SensorContext sensorContext, String path);
• private InputFile getInputFileTryRealPath(SensorContext sensorContext, String path);

to the base class of all sensors: CxxReportSensor. So each sensor could potentially benefit from this functionality. The only thing, which is hard to implement in an elegant way is the clean-up. An easy solution would be to provide a method

void CxxReportSensor::cleanNotFoundFiles();

But there will be always a risk, that someone forgets to call it. A better solution would be to implement some kind of "template method" (aka. "private interface") pattern. Like

public class CxxReportSensor implements Sensor {

private final Set<String> notFoundFiles = new HashSet<>();

protected abstract executeImpl(SensorContext context);

@Override
public void execute(SensorContext context) {
    notFoundFiles.clear();
    executeImpl(context);
}

}

All derived classes will be forced to override executeImpl() instead of execute(), but there will be no risk of inconsistency anymore.

[jmecosta]

@ivangalkin inst each sensor a own instance during the analysis stage. Even in a multi module scenario, each sensor is instantiated again and again? so the notFoundFiles is always empty when the sensor executes?

If thats the case, maybe notFoundFiles could be injected in similar way we handle the coverage cache data. It makes sense since notFoundFiles should be constant for all sensor, so it should speed up the analysis?

[ivangalkin]

@jmecosta before #1522, all sensors were derived from CxxReportSensor and all of them had a copy of notFoundFiles. Unfortunately the logic of the methods CxxReportSensor::execute(), CxxReportSensor::executeReport() etc. was absolutely not applicable to the coverage sensors (among others). So I had to create a new class CxxIssuesReportSensor (CxxIssuesReportSensor extends CxxReportSensor) and move a large block of code to this new file. But if parts of logic became useful for all sensors again, why not to move these logic back? I prefer consistency over optimization so I personally don't see any problem.

If thats the case, maybe notFoundFiles could be injected in similar way we handle the coverage cache data.

This could bring some optimization indeed. But again 1) the handling of notFoundFiles should be available to all sensors (and therefore moved back to CxxReportSensor) 2) we'll introduced some shared state between sensors. Right now such shared state doesn't play any role (all sensors are executed successively), but it could hinder the future parallelization.

[jmecosta]

So, for now just move the thing back to CxxReportSensor and leave optimization to some later stage?

[ivangalkin]

@jmecosta that would be the right solution in my opinion;

[guwirth]

@jmecosta agree with @ivangalkin

[guwirth]

@jmecosta how should we continue here? Do you make another PR with the proposal of @ivangalkin?

[guwirth]

replaced by #1701