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

Fix parsing of clang warnings with "C/C++:" prefix #1021

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

DennisBauer
Copy link
Contributor

@DennisBauer DennisBauer commented Feb 7, 2024

The prefix was detected as part of the file name
leading to a java.nio.file.InvalidPathException
in IssueBuilder.isAbsolutePath().
In addition to fixing the path parsing in ClangParser
the Exception thrown in the IssueBuilder.isAbsolutePath()
is also catched.

Testing done

Submitter checklist

Preview Give feedback

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (daece32) 89.71% compared to head (3460211) 89.68%.
Report is 9 commits behind head on main.

Files Patch % Lines
...main/java/edu/hm/hafner/analysis/IssueBuilder.java 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1021      +/-   ##
============================================
- Coverage     89.71%   89.68%   -0.03%     
  Complexity     2284     2284              
============================================
  Files           354      354              
  Lines          6690     6692       +2     
  Branches        698      698              
============================================
  Hits           6002     6002              
- Misses          486      488       +2     
  Partials        202      202              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@uhafner uhafner added the bug Bugs or performance problems label Feb 7, 2024
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The prefix was detected as part of the file name leading to a java.nio.file.InvalidPathException in IssueBuilder.isAbsolutePath().

Can you please also expose this bug in a test for IssueBuilder and then catch this Exception in IssueBuilder and return false?

@DennisBauer
Copy link
Contributor Author

I was also thinking about this but it's not clear what to return for isAbsolutePath(). Returning false will treat it as relative path, potentially adding a prefix path, which will probably make it fail somewhere else.

@uhafner
Copy link
Member

uhafner commented Feb 7, 2024

I was also thinking about this but it's not clear what to return for isAbsolutePath(). Returning false will treat it as relative path, potentially adding a prefix path, which will probably make it fail somewhere else.

Well, you can also return true if you think that would fit better. But in all other places invalid paths are catched and ignored.

@DennisBauer
Copy link
Contributor Author

If we cannot check whether the path is an absolute path or not neither returning true nor false makes sense to me. But if you're saying that the invalid path is also catched at other places I can do the same in isAbsolutePath() and return true.

The prefix was detected as part of the file name
leading to a java.nio.file.InvalidPathException
in IssueBuilder.isAbsolutePath().
In addition to fixing the path parsing in ClangParser
the Exception thrown in the IssueBuilder.isAbsolutePath()
is also catched.
uhafner added a commit to uhafner/codingstyle that referenced this pull request Feb 8, 2024
Use URI to decode URI encoded absolute paths.
See jenkinsci/analysis-model#1021
@DennisBauer DennisBauer requested a review from uhafner February 13, 2024 08:02
uhafner added a commit that referenced this pull request Feb 19, 2024
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Normally a test would be required, but since I added one in uhafner/codingstyle#921 we can ignore it right here.

@uhafner uhafner merged commit eb6427d into jenkinsci:main Feb 19, 2024
26 of 28 checks passed
@uhafner
Copy link
Member

uhafner commented Feb 19, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs or performance problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants