-
Notifications
You must be signed in to change notification settings - Fork 192
Add LlvmLinkerParser for LLVM lld linker error parsing #1245
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
base: main
Are you sure you want to change the base?
Add LlvmLinkerParser for LLVM lld linker error parsing #1245
Conversation
- Add LlvmLinkerParser class for parsing ld.lld error messages - Support versioned linkers (ld.lld-15, etc.) - Handle error/warning/note severity levels - Integrate with ClangDescriptor using composite pattern - Add comprehensive unit and integration tests Resolves JENKINS-76141
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your pull request! Just some small comments...
private static final long serialVersionUID = 1L; | ||
|
||
// Pattern that captures the linker name specifically | ||
private static final String LLD_LINKER_PATTERN = "^.*[/\\\\]?(ld\\.lld(?:-\\d+)?):\\s*(error|warning|note):\\s*(.*)$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use names for capturing groups? This would make the pattern more readable for future changes: https://jdriven.com/blog/2020/04/Java-Joy-Using-Named-Capturing-Groups-In-Regular-Expressions
.buildOptional(); | ||
} | ||
|
||
private Severity mapPriority(final String severity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an automatic mapping in Severity#guessSeverity (it maps errors to Severity.ERROR though). But if you want to map errors to severity HIGH then your code is just fine.
/** | ||
* A parser for LLVM lld linker warnings and errors. | ||
* | ||
* @author [Your Name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either your name or remove the lines:
* @author [Your Name] | |
* @author Steven Scheffler |
/** | ||
* Tests the class {@link LlvmLinkerParser}. | ||
* | ||
* @author [Your Name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @author [Your Name] | |
* @author Steven Scheffler |
assertThat(warnings.stream().allMatch(issue -> | ||
issue.getFileName().endsWith("ld.lld"))) | ||
.isTrue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally in AssertJ one would use:
assertThat(warnings.stream())
.allSatisfy(issue -> assertThat(issue).hasFileName("ld.lld"));
But this is semantically equivalent...
@sgscheffler ping: I'm not sure if you have enabled your notifications... |
Summary
Add LlvmLinkerParser for LLVM lld linker error parsing
Changes
Resolves JENKINS-76141
Testing Done
Unit Tests
LlvmLinkerParserTest
with 7 test methods covering:Integration Tests
shouldFindAllClangAndLldIssues
to ParsersTestManual Testing
mvn clean test
passesVerification
Submitter Checklist