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

Adding mergerules command to generate a merged rule file for Clang rules #2382

Merged
merged 1 commit into from
May 31, 2022

Conversation

aallrd
Copy link

@aallrd aallrd commented May 30, 2022

Hello,

I would like to contribute this mergerules function used to create a clangtidy_merged.xml file merging the output of the generate_clangtidy_resources script:

  • clangtidy_new.xml (clang-tidy rules)
  • diagnostic_new.xml (clang diagnostics rules)

It was mentioned in #2378 that this process was manual and I needed a way to have it automated.


This change is Reviewable

@guwirth
Copy link
Collaborator

guwirth commented May 31, 2022

Hi @aallrd,

thanks for your contribution, I will have a look to it.

Regards,

@guwirth
Copy link
Collaborator

guwirth commented May 31, 2022

The automated reading and creation of new rule files cannot be fully automated. Manual corrections are often necessary for the final version of the rule files. The automatically created file "clangtidy_merged.xml" can therefore probably not be used 1:1.

@aallrd Or what experience have you made?

@aallrd
Copy link
Author

aallrd commented May 31, 2022

I compared the list of clangtidy rules (keys) from three files:

  • The clangtidy.xml from the 2.0.6.2925 release JAR (sonar-cxx-plugin-2.0.6.2925.jar): 1316 rules
    • Sourced from llvmorg-14-init-8123-ga875e6e1225a
  • The clangtidy.xml from the 2.0.7.3119 release JAR (sonar-cxx-plugin-2.0.7.3119.jar): 1340 rules
    • Sourced from llvmorg-15-init-2831-geb3e09c9bf1d
  • The clangtidy_merged.xml built using the scripts from my custom LLVM/Clang sources: 1347 rules

Here is the diff of rules between the 2.0.7.3119 release JAR and my clangtidy_merged.xml:

+     <key>abseil-cleanup-ctad</key>
+     <key>misc-unused-function</key>
+     <key>misc-unused-variable</key>
+     <key>modernize-deprecated-bind1st</key>
-     <key>bugprone-shared-ptr-array-mismatch</key>
-     <key>clang-diagnostic-always-inline-coroutine</key>
-     <key>clang-diagnostic-requires-expression</key>
-     <key>clang-diagnostic-return-std-move</key>
-     <key>clang-diagnostic-unaligned-access</key>
-     <key>clang-diagnostic-unqualified-std-cast-call</key>
-     <key>clang-diagnostic-unsupported-abi</key>
-     <key>misc-misleading-bidirectional</key>
-     <key>readability-container-contains</key>
-     <key>readability-data-pointer</key>
-     <key>readability-duplicate-include</key>

The added misc and modernize are expected because we kept them in our LLVM/Clang fork while they are gone upstream.
The missing ones seem to have been added somewhere in the past ~4 months and our fork is a bit older than that,

From this comparison I would say the function did a correct job merging the files and created a valid and suitable clangtidy_merged.xml for production.

What are the manual corrections you are doing on your side on the clangtidy.xml file?

@guwirth
Copy link
Collaborator

guwirth commented May 31, 2022

@aallrd in the past I had to adapt some keys and descriptions manually after creating xml files from a new Clang version. After executing your script and comparing the result with clangtidy.xml you should see the differences.

Your script extension is definitely an improvement and I will merge it. Thanks again for that!
What I doubt is if you can run everything automated?

@guwirth guwirth merged commit fe02893 into SonarOpenCommunity:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants