-
IntroductionTo improve the code quality of Autoware, we are currently using clang-tidy on GitHub CI. This is a similar proposal with https://github.com/orgs/autowarefoundation/discussions/4827. Current progressWe 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. ProposalWe propose the following two things:
I recommend to enable the following features, based on the analysis above:
Future PlanWe will first solve the "CRITICAL" level warnings since these prevent clang-tidy from working well. Request for FeedbackIf you have any comments or suggestions regarding this proposal, please feel free to provide your feedback! |
Beta Was this translation helpful? Give feedback.
Replies: 11 comments 55 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.
@xmfcx
Now, the original purpose "enable clang-tidy as a required CI" has been achived! 🎉
We are now under step 5:
Add a new warning to .clang-tidy-ci which we want to make required
, but this is an endless work and there is an issue to discuss which checks should be required or not.May I close this discussion as completed?