Replies: 11 comments 50 replies
-
@veqcc Could you provide some examples on what kind of critical level warning we have to give us some idea? |
Beta Was this translation helpful? Give feedback.
-
We have 40~50 CRITICAL level warnings, and they are classified as the following 9 type warnings:
hoge and fuga are used to abstract actual function or variable names. |
Beta Was this translation helpful? Give feedback.
-
@mitsudome-r @xmfcx So I created a PR to reduce the clang-tidy checkings so that |
Beta Was this translation helpful? Give feedback.
-
@veqcc |
Beta Was this translation helpful? Give feedback.
-
@veqcc
I would be happy for your thoughts on this! |
Beta Was this translation helpful? Give feedback.
-
@xmfcx In the current clang-tidy configuration, Line 203 in ada7b8c However, it seems not working propoerly. For example, in this PR, clang-tidy reports errors in /usr directories.
I'll make a deeper investivation later. |
Beta Was this translation helpful? Give feedback.
-
@xmfcx @mitsudome-r After that, I made some investigations on how to solve the problems we have encoutnered, and found some solutions
Although not all the problems are solved yet, it is also true that several Autowarwe packages already pass clang-tidy checks.
The whole picture of this proposal look like the following: If you have any comments or suggestions on this proposal, please provide your feedback by 25th of November (next Monday). |
Beta Was this translation helpful? Give feedback.
-
@xmfcx @mitsudome-r
|
Beta Was this translation helpful? Give feedback.
-
About modernize-pass-by-value: As @soblin mentioned internally, this is unnecessarily harsh to const-refs, or more precisely, its proposed fix is not applicable to all situations, and thus quite reckless. We are putting it into the disabled checks list. |
Beta Was this translation helpful? Give feedback.
-
As described in #5520 and discussed internally, can we open a discussion regarding more strict static check configuration? The list of future additional checks is in linked issue, however I would like to ask community which configurations should be prioritized first. Personally, I think cppcoreguidelines-narrowing-conversions is bug prone, occurs frequently in Autoware and might be our next step towards code quality improvement. Looking forward for more suggestions. |
Beta Was this translation helpful? Give feedback.
-
Here are the stats for different Clang-Tidy diagnostics (all linked analyses were enabled): The stats
When only showing
|
Beta Was this translation helpful? Give feedback.
-
Introduction
To improve the code quality of Autoware, we are currently using clang-tidy on GitHub CI.
However, the results of clang-tidy are always discarded and all PRs are merged with warnings, which leads to 15,000 warnings of clang-tidy for Autoware.
My proposal is narrowing down the clang-tidy check features (written in
.clang-tidy
file) and making it required to pass all checks in each PR.This is a similar proposal with https://github.com/orgs/autowarefoundation/discussions/4827.
Current progress
We have analyzed the current 15,000 clang-tidy warnings and classified them based on codechecker criteria.
"C" is "CRITICAL" level warnings, which means it failed to compile with clang.
"H" is "HIGH" level warnings. Out of 107 "HIGH" features prepared in clang-tidy, the current Autoware enables 18 features, and 6 of them offer some warnings. In other words, Autoware passes 12 features of them.
"M" is "MIDIUM" level warnings. Out of 1098 "MIDIUM" features prepared in clang-tidy, the current Autoware enables 40 features, and 13 of them offer some warnings. In other words, Autoware passes 27 features of them.
"L", "S" and "U" are "LOW", "STYLE" and "UNKNOWN", respectively. I will ignore these low level warnings this time.
You can find all clang-tidy warnings and the features currencly enabled in Autoware.
Proposal
We propose the following two things:
I recommend to enable the following features, based on the analysis above:
Future Plan
We will first solve the "CRITICAL" level warnings since these prevent clang-tidy from working well.
After that, we will move on to removing "HIGH" and "MIDIUM" level warnings which the current Autoware cannot pass.
When we succeed to remove all the warnings related a feature among them, we want to add the feature to check list and make it required.
Request for Feedback
If you have any comments or suggestions regarding this proposal, please feel free to provide your feedback!
Beta Was this translation helpful? Give feedback.
All reactions