-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add Clang-Tidy converter #2367
Add Clang-Tidy converter #2367
Conversation
src/ReleaseHistory.md
Outdated
@@ -3,6 +3,7 @@ | |||
## **v2.4.9** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.9) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.9) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.9) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.9) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.9) | |||
* FEATURE: Report inner exception details if available. [#2357](https://github.com/microsoft/sarif-sdk/pull/2357) | |||
* FEATURE: Add support for git blame. [#2358](https://github.com/microsoft/sarif-sdk/pull/2358) | |||
* FEATURE: Add Clang-Tidy converter. [#2367](https://github.com/microsoft/sarif-sdk/pull/2367) |
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.
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.
got it, fixed
|
||
Result result = new Result() | ||
{ | ||
RuleId = entry.DiagnosticName, |
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.
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.
each rule can point to its own documentation: https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-trailing-return-type.html
change modernize-use-trailing-return-type to another ruleid and it will work
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.
added the rules.
{ | ||
"physicalLocation": { | ||
"artifactLocation": { | ||
"uri": "/home/lsp/projects/staticlib/src/Hello.cpp", |
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.
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.
you mean the uri is not needed here because the url is already in the artifacts section, since we have the index here, we can know the url by looking at artifacts section by index right.
this is same as LintConverter, it is also duplicating this info,
the index is automaticlly added by code, once we specifc the url here. I think ideally the code that automaticlly add index can delete the url at the same time, if we want to do that. let me know if we want make changes.
This pull request introduces 1 alert when merging 3eea2e5 into 60363e6 - view on LGTM.com new alerts:
In reply to: 861791781 |
private static (List<ReportingDescriptor>, List<Result>) ExtractRulesAndResults(ClangTidyLog log) | ||
{ | ||
var rules = new List<ReportingDescriptor>(); | ||
var ruleIds = new HashSet<string>(); |
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.
u could replace by dict<string, reportingdescriptor> #Resolved
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.
add the stringcomparison to ignore case
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.
sounds good, fixed
DefaultConfiguration = new ReportingConfiguration | ||
{ | ||
Level = FailureLevel.Warning, | ||
}, |
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.
I think u dont need to add this. #Resolved
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.
sorry did not get you, removing
},
will have compile error
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.
remove entire DefaultConfiguration
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.
oh do you mean remove the whole DefaultConfiguration since no need. I am fixing it.
DefaultConfiguration = new ReportingConfiguration
{
Level = FailureLevel.Warning,
},
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.
fixed
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.
warning is default. so we dont need to express.
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.
No description provided.