fail `run-clang-tidy` in case of compiler warnings / fixed Clang warnings / cleanups #4036

firewave · 2022-04-21T07:30:21Z

clang-tidy will also report compiler warnings which we were currently suppressing since we treat all warnings as errors.

We should make sure our code is free of warnings. So we should fail the build in the CI (not by default) if one is encountered.

Leveraging the existing clang-tidy step saves us an additional explicit step in the CI for a clang build.

firewave · 2022-04-21T07:31:48Z

lib/importproject.cpp

 {
    std::set<std::string> found;
+    // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
    const std::list<std::string> copyIn(in);


@danmar Why do you need to copy this?

to allow that includePaths is passed as in parameter

Wait...what? That took me quite a while to understand... We should not do that but let's tackle that at a later date.

firewave · 2022-04-21T07:40:20Z

cmake/clang_tidy.cmake

 if (RUN_CLANG_TIDY)
    # disable all compiler warnings since we are just interested in the tidy ones
-    add_custom_target(run-clang-tidy ${RUN_CLANG_TIDY} -checks=-performance-unnecessary-copy-initialization -p=${CMAKE_BINARY_DIR} -j ${NPROC} -extra-arg=-w -quiet)
+    add_custom_target(run-clang-tidy ${RUN_CLANG_TIDY} -p=${CMAKE_BINARY_DIR} -j ${NPROC} -extra-arg=-w -quiet)


@danmar This would have affect all source files.
Interestingly this is also something we check for so the self-check should have detected this as well.

It's also possible the -checks option might override the existing .clang-tidy configuration - which would be a bad thing.

It's also possible the -checks option might override the existing .clang-tidy configuration - which would be a bad thing.

Actually it is appended:

--checks=<string> - [...] This option's value is appended to the value of the 'Checks' option in .clang-tidy file, if any.

firewave · 2022-04-21T17:02:35Z

I created llvm/llvm-project#55018 for the -Wmissing-prototypes warning in the fuzzer.

firewave · 2022-04-29T20:33:36Z

This is ready for review.

danmar

there are changed ts files. please revert those changes.

firewave · 2022-04-30T05:53:33Z

there are changed ts files. please revert those changes.

Those are changed since I changed GUI source files.

danmar · 2022-04-30T06:07:19Z

I do not feel I can review this PR properly since there are so many changed ts files. please revert. I suggest separate update of ts files. I think the CI needs to be less strict if it cant handle this.

firewave · 2022-04-30T06:28:06Z

I do not feel I can review this PR properly since there are so many changed ts files. please revert.

Please review by looking at the separate commits instead of the changed files. It's all very small changes which are very easy to review. The translation changes are always a separate commit to make it easier to review (even though they should be combined with the actual source changes but since we squash anyways there's no point in that).

I suggest separate update of ts files. I think the CI needs to be less strict if it cant handle this.

There's an option to remove the line numbers but we need to test what the effects of this is since it is not clearly documented (apparently it is necessary if you use the QtLinguist UI for exiting them). I would prefer to do this at the beginning of a release.

danmar · 2022-04-30T06:56:31Z

It seems better that we leave the ts files unchanged for some periods. no need to update them in every PR.
I used to update the ts files during each release I think that is better.

firewave · 2022-04-30T15:40:29Z

It seems better that we leave the ts files unchanged for some periods. no need to update them in every PR.
I used to update the ts files during each release I think that is better.

Okay - so I will no longer commit them in my upcoming changes.

The problem of them being outdated is only felt in the non-CMake builds (i.e. qmake) since those are not updating them implicitly like out CMake build does.

The changes to them in this PR are much smaller than many of the recent one so I think it should be fine to still change them this once since the CI is already green.

* Fix #11041 FN constVariable with array of pointers [regression] * Use std::vector for deterministic order of results, use helper variables

danmar · 2022-05-08T18:39:31Z

The changes to them in this PR are much smaller than many of the recent one so I think it should be fine to still change them this once since the CI is already green.

I don't understand why you don't revert the changes now. There is a release quite soon and they will be updated then.

firewave · 2022-05-08T19:00:00Z

I don't understand why you don't revert the changes now. There is a release quite soon and they will be updated then.

I dropped it.

firewave · 2022-05-09T20:50:18Z

This is fine now and ready to be reviewed again.

… an additional CI step

…have the proper warning flags

firewave changed the title ~~also fail clang-tidy in case of a compiler warning / fixed Clang warnings~~ also fail clang-tidy in case of a compiler warning / fixed Clang warnings / cleanups Apr 21, 2022

firewave commented Apr 21, 2022

View reviewed changes

firewave changed the title ~~also fail clang-tidy in case of a compiler warning / fixed Clang warnings / cleanups~~ fail run-clang-tidy in case of compiler warnings / fixed Clang warnings / cleanups Apr 21, 2022

firewave commented Apr 21, 2022

View reviewed changes

firewave force-pushed the tidy-warn branch 4 times, most recently from f546e8a to 345e5b5 Compare April 21, 2022 17:01

firewave marked this pull request as draft April 21, 2022 17:03

firewave force-pushed the tidy-warn branch from 345e5b5 to b7c2916 Compare April 21, 2022 17:13

firewave marked this pull request as ready for review April 21, 2022 17:13

firewave marked this pull request as draft April 22, 2022 08:16

firewave force-pushed the tidy-warn branch from b7c2916 to a0e5630 Compare April 29, 2022 20:29

firewave marked this pull request as ready for review April 29, 2022 20:33

danmar reviewed Apr 30, 2022

View reviewed changes

firewave referenced this pull request May 6, 2022

Fix #11041 FN constVariable with array of pointers [regression] (#4080)

aebc080

* Fix #11041 FN constVariable with array of pointers [regression] * Use std::vector for deterministic order of results, use helper variables

firewave force-pushed the tidy-warn branch from a0e5630 to 75cf37c Compare May 8, 2022 18:59

firewave force-pushed the tidy-warn branch from 75cf37c to 853688f Compare May 9, 2022 20:40

firewave mentioned this pull request May 11, 2022

updated CI to latest ubuntu and clang #4004

Merged

properly suppress clang-tidy warning in importproject.cpp

77fb49b

firewave added 12 commits May 14, 2022 19:09

clang_tidy.cmake: fail if any compiler warning is detected - saves us…

ee7e246

… an additional CI step

suppressions.h: fixed -Wdocumentation warning

9e584d2

fixed -Wshadow warning for testcppchecklibrarydata.cpp::loadCfgFile()

2c12f18

config.h: added UNUSED macro

e88de11

resultstree.cpp: removed unused function

ebc792e

translationhandler.cpp: avoid -Wunused-function warning

35cd258

resultstree.cpp: avoid -Wdeprecated-declarations warning

afd595b

removed some unnecessary __cplusplus preprocessor checks

81efa0f

config.h: made SUPPRESS_WARNING usable by any code

a77ef38

clang-tidy.yml: use clang to generate the compilation database so we …

836c8f4

…have the proper warning flags

oss-fuzz/main.cpp: fixed -Wmissing-prototypes warning

2be9107

astutils.h: fixed -Wdocumentation warning

3de0310

firewave force-pushed the tidy-warn branch from 853688f to 3de0310 Compare May 14, 2022 17:10

danmar merged commit 14421ae into danmar:main May 15, 2022

firewave deleted the tidy-warn branch May 15, 2022 12:04

firewave mentioned this pull request May 20, 2022

Improved documentation of supported toolchains #3596

Draft

fail run-clang-tidy in case of compiler warnings / fixed Clang warnings / cleanups #4036

fail run-clang-tidy in case of compiler warnings / fixed Clang warnings / cleanups #4036

Uh oh!

Conversation

firewave commented Apr 21, 2022

Uh oh!

firewave Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

danmar Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

firewave Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

firewave Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

firewave Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

firewave May 15, 2022

Choose a reason for hiding this comment

Uh oh!

firewave commented Apr 21, 2022

Uh oh!

firewave commented Apr 29, 2022

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

firewave commented Apr 30, 2022

Uh oh!

danmar commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

firewave commented Apr 30, 2022

Uh oh!

danmar commented Apr 30, 2022

Uh oh!

firewave commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar commented May 8, 2022

Uh oh!

firewave commented May 8, 2022

Uh oh!

firewave commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fail `run-clang-tidy` in case of compiler warnings / fixed Clang warnings / cleanups #4036

fail `run-clang-tidy` in case of compiler warnings / fixed Clang warnings / cleanups #4036

danmar commented Apr 30, 2022 •

edited

Loading

firewave commented Apr 30, 2022 •

edited

Loading