Skip to content
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

Implement support for multiple locations in CxxReportSensor #1447

Merged

Conversation

ivangalkin
Copy link
Contributor

@ivangalkin ivangalkin commented Apr 2, 2018

enable multiple locations for

  • CxxDrMemorySensor
  • CppcheckParserV2
  • CxxValgrindSensor
    enable the new interface for all remaining sensors

this pull request is replacement for #1436
see also issue #1440


This change is Reviewable

}
}
}

private void addFileLocations(final SensorContext context, DrMemoryError error, List<CxxReportLocation> locations) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same logic is implemented in CxxValgrindSensor

For each CxxReportLocation CxxReportSensor will check if path belongs to the project or not. If it doesn't - this [secondary] location will be ignored. So finally we will have

  • a primary location which is guaranteed to belong to the project (same as before)
  • >= 0 additional locations, which reference files from the analyzed project;

/**
* Issue with one or multiple locations
*/
public class CxxReportIssue {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no logic here - CxxReportIssue and CxxReportLocation (1:n) were introduced more or less in order to implement the proper semantics of ::equals() and ::hashCode() methods

Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivangalkin thanks for your update.

Think interface would be easier to use if you would move CxxReportLocation inside of CxxReportIssue.

First issue with location would be with ctor:

 // instead of this:
  public CxxReportIssue(String ruleRepoKey, String ruleId, CxxReportLocation primaryLocation) {
    this(ruleRepoKey, ruleId, Collections.singletonList(primaryLocation));
  }

 // this one for first issue/location:
  public CxxReportIssue(String ruleRepoKey, String ruleId,
                                    String file, String line, String info) {
       super();
       this.ruleRepoKey = ruleRepoKey;
       this.ruleId = ruleId;
       addLocation(file, line, info)
  }

 // remove this one
  public CxxReportIssue(String ruleRepoKey, String ruleId, List<CxxReportLocation> locations) {
    super();
    this.ruleRepoKey = ruleRepoKey;
    this.ruleId = ruleId;
    this.locations = locations;
}

Second locations with method:

   void addLocation(String file, String line, String info) {
      locations.add(CxxReportLocation(file, line, info));
   }

There would be no need for users (sensors) of the interface to create and handle own CxxReportLocation.

What do you think?

import javax.xml.stream.XMLStreamException;
import org.codehaus.staxmate.in.SMHierarchicCursor;
import org.codehaus.staxmate.in.SMInputCursor;
import org.sonar.api.batch.sensor.SensorContext;
import org.sonar.api.utils.log.Logger;
import org.sonar.api.utils.log.Loggers;
import org.sonar.cxx.sensors.utils.CxxReportIssue;
import org.sonar.cxx.sensors.utils.CxxReportLocation;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this empty statement ";;"

String line = null;
String id = Objects.requireNonNull(errorCursor.getAttrValue("id"),
"Missing mandatory attribute /results/errors/error[@id]");
String msg = Objects.requireNonNull(errorCursor.getAttrValue("msg"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this useless assignment to local variable "msg"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx! done

@ivangalkin
Copy link
Contributor Author

@guwirth Thank you for your proposal. I started to implement the CxxReportIssue in the way you suggested. However the existing code is very careful w.r.t. the XML (or other source) validation, so we save an issue only if at least 1 valid location exists. This makes the generic logic work like that:

  1. inspect and collect all possible locations
  2. if there is at least 1 valid one -> save the issue

Creation of an CxxReportIssue in advance is often impossible or makes the logic more complex.

Also there is a (minor) performance aspect (or rather memory footprint). Since the most of sensors support only primary location we can benefit from the usage of Collections.singletonList(). The suggested approach would always require a full-blown container.

@ivangalkin
Copy link
Contributor Author

@Bertk please review manually, GitHub doesn't notice, that I've changed the SonarQube finding. I've fixed an issue, but in an other line as reviewed.

@guwirth
Copy link
Collaborator

guwirth commented Apr 5, 2018

@ivangalkin

if there is at least 1 valid one -> save the issue

No location means issue on module/project level.

Creation of an CxxReportIssue in advance is often impossible or makes the logic more complex.

Maybe ctor without location and only this addLocation method? If you need an extra check why not add an isValid method to CxxReportIssue?

@ivangalkin
Copy link
Contributor Author

@guwirth I'll try to apply the comments in the next few days.

* CxxDrMemorySensor
* CppcheckParserV2
* CxxValgrindSensor
* enable the new interface for all remaining sensors

see also issue SonarOpenCommunity#1440
Copy link
Collaborator

@guwirth guwirth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @Bertk what do you think?

@guwirth guwirth merged commit 1d7e9de into SonarOpenCommunity:master May 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants