Fix clang tidy on ci

[ottml]

The ci uses clang and clang tidy packages version 18 but the scripts call the old clangtidy version 10. The clang tidy step succeds but the checks are never executed because clang tidy is not found.

Requires:

• Automatic fixes of clang-tidy warnings: misc-unused-using-decls,moder… s25edit#36
• Automatic fixes of clang-tidy warnings: misc-unused-using-decls,moder… libsiedler2#26
• Automatic fixes of clang-tidy warnings: misc-unused-using-decls,moder… libendian#5
• Automatic fixes of clang-tidy warnings: readability-simplify-boolean-… libutil#33
• Fix clang tidy on ci libutil#34
• Automatic fixes of clang-tidy warnings: misc-unused-using-decls,moder… mygettext#10
• Automatic fixes of clang-tidy warnings: readability-redundant-inline-… libutil#35

[Flamefire]

Ouch, that is indeed a major omission.

Can we extend the CI check to a) get the version from CMAKE_CXX_COMPILER and b) check the availability of the run-script first? I'm surprised that didn't lead to a failure before.

As for the current failures:

• Why is /usr/include/boost/bind/mem_fn_template.hpp: reported? Shouldn't that be excluded by -header-filter or is this some thing that changed?
• readability-identifier-length, performance-enum-size and bugprone-easily-swappable-parameters can be deactivated.
• I'm preparing a PR for modernize-type-traits
• The unit test CI will be fixed by Fix size of Settings window #1750 and related PRs once @Flow86 has time to merge them

[ottml]

We have the following erros with clang-tidy 18

The question is what we want to do with these ones. Disable all of it for the moment or some of it. Or do we want to fix it?

Some of the errors can be fixed by clang tidy automatically.

Summary: 10608 Errors

[bugprone-assignment-in-if-condition] 19
[bugprone-casting-through-void] 1
[bugprone-empty-catch] 9
[bugprone-exception-escape] 3
[bugprone-implicit-widening-of-multiplication-result] 89
[bugprone-optional-value-conversion] 2
[bugprone-reserved-identifier] 47
[bugprone-switch-missing-default-case] 105
[bugprone-unchecked-optional-access] 1
[bugprone-unhandled-exception-at-new] 1
[clang-analyzer-core.NonNullParamChecker] 1
[clang-analyzer-cplusplus.NewDeleteLeaks] 1
[clang-analyzer-optin.core.EnumCastOutOfRange] 11
[misc-confusable-identifiers] 217
[misc-const-correctness] 1693
[misc-include-cleaner] 7329
[misc-no-recursion] 54
[misc-unused-using-decls] 1
[misc-use-anonymous-namespace] 152
[modernize-macro-to-enum] 24
[modernize-use-auto] 2
[performance-avoid-endl] 134
[performance-inefficient-vector-operation] 1
[performance-move-const-arg] 2
[performance-no-int-to-ptr] 6
[performance-noexcept-swap] 3
[readability-avoid-nested-conditional-operator] 66
[readability-avoid-return-with-void-value] 9
[readability-container-data-pointer] 37
[readability-container-size-empty] 7
[readability-function-cognitive-complexity] 351
[readability-redundant-casting] 15
[readability-redundant-inline-specifier] 51
[readability-redundant-member-init] 3
[readability-simplify-boolean-expr] 12
[readability-suspicious-call-argument] 44
[readability-use-anyofallof] 30

[Flamefire]

Great work, thanks!

I'd like to check them first and see which ones to fix. As they are new I can't tell off the top of my head which ones are "good".

First we'd need to make sure /usr/include and external/kaguya|turtle get excluded as intended. That should squash a great deal of warnings already.
Could you look into that?

A few can be disabled right away:

• bugprone-switch-missing-default-case
• readability-identifier-length
• performance-enum-size
• bugprone-easily-swappable-parameters
• bugprone-implicit-widening-of-multiplication-result
• performance-avoid-endl
• Not sure about bugprone-assignment-in-if-condition as it looks reasonable

For readability-function-cognitive-complexity I would set IgnoreMacros=false and Threshold to something high enough after inspecting the results with macros ignored (assert etc adds complexity which is irrelevant)

Can you remove the last commit such that the new additions are clearly visible? And what is the command to auto-fix issues if you have it at hand? E.g. the const-correctness check looks pretty much made for that and I'm a big fan of it.

[ottml]

To apply fixes if available you can use the following parameters to the clang tidy call

 --export-fixes=<filename>        - YAML file to store suggested fixes in. The
                                     stored fixes can be applied to the input source
                                     code with clang-apply-replacements.
  --fix                            - Apply suggested fixes. Without -fix-errors
                                     clang-tidy will bail out if any compilation
                                     errors were found.

[ottml]

First we'd need to make sure /usr/include and external/kaguya|turtle get excluded as intended. That should squash a great deal of warnings already.
Could you look into that?

I had a look into it. In general the exclude header regex is working. But there are some checks ignoring it like [misc-confusable-identifiers]. I think this is due to the nature of the checks. Even with a newer clang-tidy version 19, where a new parameter

   --exclude-header-filter=<string> - Regular expression matching the names of the>                                      headers to exclude diagnostics from. Diagnostics
                                      from the main file of each translation unit are
                                      always displayed.

is introduced, the errors for these specific checks are reported, even if the according headers are excluded.

I think the part

Diagnostics from the main file of each translation unit are always displayed.

is important here.

I made also a very simple example to check it. With the check misc-confusable-identifiers it is not working and with the check google-explicit-constructor all is working fine. So we have no other possibilty as to disable these checks, or write a bug/question issue at the clang-tidy git project, to get an answer why the warnings generated by the excluded headers for this checks are not excluded.

[Flamefire]

So we have no other possibilty as to disable these checks, or write a bug/question issue at the clang-tidy git project, to get an answer why the warnings generated by the excluded headers for this checks are not excluded.

I'd say both. They shouldn't disallow identifiers in a project due to identifiers in an unrelated project

[ottml]

Opened a bug report at the clang-tidy project llvm/llvm-project#142112

[ottml]

Opened a bug report at the clang-tidy project llvm/llvm-project#142112

We have to disable this test until the option for ignoring IgnoreSystemIdentifiers is implemented by clang-tidy.

[Flamefire]

Agreed. I think we can also disable misc-include-cleaner (doesn't allow header collections such as boost/test/unit_test.hpp for BOOST_TEST), misc-use-anonymous-namespace (not that useful); misc-no-recursion (false positives for intended usage)

readability-simplify-boolean-expr needs IgnoreMacros = true

That should bring the numbers down by a lot. Maybe you can strike through the already disabled ones in your list above

[ottml]

I would fix the following warnings in this pullrequest because theses checks are already active in clang tidy 10. But after this checks fixed i would prefer to disable the other new ones temporarly. Merge this request so no more regressions can come in because of clang tidy not running. Make a new issue for reactivating/disable and fixing the new ones.

misc-unused-using-decls
modernize-use-auto
performance-inefficient-vector-operation
performance-move-const-arg
readability-container-size-empty
readability-redundant-member-init
bugprone-exception-escape

[Flamefire]

That would be great! Could you also fix performance-noexcept-swap, readability-redundant-inline-specifier and readability-container-data-pointer? The latter 2 should be possible as an automatic fix, the other only has 3 occurrences.

[ottml]

@Flamefire How can i commit the changes on the submodules without forking all of them?

[Flamefire]

@Flamefire How can i commit the changes on the submodules without forking all of them?

You can't. You'd need to fork, commit, push and PR them as usual. If you want you can attach the patch here and I'll do that. Run git format-patch -1 HEAD in the submodule folder to export one commit (-1, i.e. the most recent) as a patch which one can apply including the metadata, e.g. your authorship.

[ottml]

@Flamefire Everything is fixed. Only two modernize-type-traits errors are left but i have not idea how to fix this or if it is a false positive of clang-tidy. Can you have a look at it?

Afterwards the pullrequest for the other repos can merged and we can adjust the externals here for final testing.

[Flamefire]

Only two modernize-type-traits errors are left but i have not idea how to fix this or if it is a false positive of clang-tidy. Can you have a look at it?

Yes it's a bug which seems to be fixed in at least clang-tidy trunk: https://godbolt.org/z/dco63azYW
so just ignore with a comment

Afterwards the pullrequest for the other repos can merged and we can adjust the externals here for final testing.

Can you link them in the description? Usual is something like:

Requires:
- [ ] <link to pr>

GitHub then automatically shows/updates the merge state

[Flamefire]

What about the libutil PR?

BTW: If you rebase this could you change the commit message of e.g. "Deactivate clang-tidy modernize-type-traits due to bug in clang-tidy" to something like "suppress false positive of clang-tidy modernize-type-traits due to bug in clang-tidy"? Currently it sounds like it disabled the test for everything which might be misleading later.
Just if you touch the history anyway.

[ottml]

What about the libutil PR?

BTW: If you rebase this could you change the commit message of e.g. "Deactivate clang-tidy modernize-type-traits due to bug in clang-tidy" to something like "suppress false positive of clang-tidy modernize-type-traits due to bug in clang-tidy"? Currently it sounds like it disabled the test for everything which might be misleading later. Just if you touch the history anyway.

I missed some place to fix in libutil so i need a new PR. Thanks for merging.

Yes sure i can change the commit message.

[Flamefire]

There was an unrelated issue in libutil

I added a (hopefully) fix. So just update the submodule here

[Flamefire]

There was an unrelated issue in libutil

I added a (hopefully) fix. So just update the submodule here

Didn't work had to revert it. We need to wait for #1778

[ottml]

Everything should be fine now :)

[Flamefire]

Thanks for your efforts!