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

5.6updates #903

Closed
wants to merge 18 commits into from
Closed

5.6updates #903

wants to merge 18 commits into from

Conversation

jmecosta
Copy link
Member

@jmecosta jmecosta commented Jul 3, 2016

No description provided.

@jmecosta
Copy link
Member Author

jmecosta commented Jul 6, 2016

@ALL this is ready for initial review. this mostly work. the only thing i cannot figure out is a parser error in the integration tests that are causing changes to several metrics. funny thing is i did not touch the grammar. @guwirth @Bertk can you try check this.

when we have a new keyword:

ERROR: Parse error at line 24 column 29:

11: int Bar::foo(){
12:
13:
14:
15:
16:
17: int
18: x;
19:
20: return 111;
21: }
22:
23:
--> void Bar::do_valgrind_errors(){
25:
26:
27:
28: __newfloat();
29:

can it be some encoding issue witht the file?

@jmecosta
Copy link
Member Author

jmecosta commented Jul 6, 2016

the sslr toolkit is able to parse that file.

@jmecosta
Copy link
Member Author

jmecosta commented Jul 6, 2016

hum, both the sslr toolkit and the cxx-linter are able to process the file without issues. for some reason it does not work during analysis. plus i in one of our projects and i get all stuff working properly

</executions>
</plugin>
</plugins>
</build>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmecosta which of the other plugins are you using as source for this pom?

Copy link
Member Author

@jmecosta jmecosta Jul 6, 2016

Choose a reason for hiding this comment

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

likely from the java plugin, one of the many tries to solve the classnotfound from the plugin. likely can be clean. i did clean most of the other ones. i will take it a try if it works. #todo

Copy link
Member Author

Choose a reason for hiding this comment

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

done. removed and working.

if (normalPath != null) {
coverageData.put(normalPath, builder); // replace id with path
}
//String normalPath = CxxUtils.normalizePath(sourceFile.getAttrValue("path"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmecosta code to delete or not finished?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@guwirth
Copy link
Collaborator

guwirth commented Jul 6, 2016

@jmecosta looks like a new plugin :-) .Thanks again for all your time...

One thing is not totally clear for me: what exactly is already working and what is for sure not working currently?

@jmecosta
Copy link
Member Author

jmecosta commented Jul 7, 2016

@guwirth tanks for the review. ive implemeted all your feedback and raise new issues for stuff that should be handled outside.

  • the plugin is mostly functional. the follwing was dropped or temporary disabled.
    . zero coverage
    . test details
    . xpath
  • there is however a critical issue with this, one parse error in the integration tests that causes all coverage metrics to fail. i tought it was related with encoding for that particular sample but i tried several without being able to get it running. The SSLR toolkit and the Cxx-Linter are able to parse it propertly. Funny thing is that ive run the plugin in a one of our internal projects that is far more complex and it works fine. i will still try to recreate the files in Visual Studio to see what it does. If you have any hints on this would be good.

Todo:

@jmecosta
Copy link
Member Author

jmecosta commented Jul 7, 2016

@guwirth found the cause of the critical error. the files that fail to parsed are ended in .cc if renaming those to .cpp they parse properly. so it must be something to the parser consider them as c and its running in compatibility mode. and the problem is this change https://github.com/SonarOpenCommunity/sonar-cxx/pull/903/files#diff-8e7776ebd58c67b7a828c983909d6fa4R290

so in order to remove the need for common.io there i broke the. reg expression there. i will fix that today. and afterwards i think it should be ok

@guwirth
Copy link
Collaborator

guwirth commented Jul 7, 2016

found the cause of the critical error. the files that fail to parsed are ended in .cc if renaming

@jmecosta maybe the problem is here:
https://github.com/SonarOpenCommunity/sonar-cxx/pull/903/files#diff-8e7776ebd58c67b7a828c983909d6fa4R290

   private boolean isCFile(String filePath) {
     for (String pattern : cFilesPatterns) {
-      if (wildcardMatchOnSystem(filePath, pattern)) {
+      String regex = pattern.replace("?", ".?").replace("*", ".*?");
+      if (filePath.matches(regex)) {
         if (LOG.isTraceEnabled()) {
           LOG.trace("Parse '{}' as C file, matches '{}' pattern", filePath, pattern);
         }

@guwirth
Copy link
Collaborator

guwirth commented Jul 7, 2016

@jmecosta maybe you can squash and create a new branch 5.6updatesV2. Think there are too many comments in this one. Why are the test on the integration server not green? What's missing?

If 5.6updatesV2 looks good and integration tests are green again I would merge it. Open/remaining points can be tracked as issues. Then also others can work in parallel on it...

@jmecosta
Copy link
Member Author

jmecosta commented Jul 7, 2016

@guwirth the integration tests are broken because of the c files thing. trying to get it running but seems those trace logs dont show up in log... wondering how is that even called

@jmecosta
Copy link
Member Author

jmecosta commented Jul 8, 2016

interation 2 - > #913

@jmecosta jmecosta closed this Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants