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

Enable clang-tidy's bugprone-reserved-identifier check #15879

Closed
wants to merge 1 commit into from

Conversation

sebproell
Copy link
Contributor

As requested in #15870 (comment)

@peterrum
Copy link
Member

clang-tidy 3.5 hours. Is that reasonable!? Normally, it takes about 1h!

Copy link
Member

@tamiko tamiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to get the tidy test to run within around 60 minutes.

@sebproell
Copy link
Contributor Author

clang-tidy 3.5 hours.

Interesting. I suppose that's the reason why this and other checks haven't been enabled so far ;)

I don't know of a way to get this faster. Close?

@blaisb
Copy link
Member

blaisb commented Aug 16, 2023

Ohh I had not spotted that the CI was failing because of a timeout. That is an absurd time...

@blaisb blaisb self-requested a review August 16, 2023 12:27
@tamiko
Copy link
Member

tamiko commented Aug 16, 2023

@blaisb It was running only for around 3h at that point 😄

@peterrum
Copy link
Member

I am wondering why this check is sooo expensive? It seems harmless. I have restarted the test to see if we were just unlucky with the worker.

@tamiko
Copy link
Member

tamiko commented Aug 17, 2023

@peterrum 3h later. Something is seriously wrong.
@sebproell Let's keep the PR open for a little while for us to figure out what the problem is.

@bangerth
Copy link
Member

@sebproell Do you have a way to check on your local machine how long the clang-tidy runs take with/without the new flag?

@peterrum
Copy link
Member

peterrum commented Sep 7, 2023

@sebproell Do you have a way to check on your local machine how long the clang-tidy runs take with/without the new flag?

@sebproell ?

@sebproell
Copy link
Contributor Author

@bangerth @peterrum

Sorry. I missed that comment. I can have a look.

@sebproell
Copy link
Contributor Author

sebproell commented Oct 24, 2023

On my machine (20 threads, 64GB RAM) using LLVM 17.2 I get for the run of clang-tidy (without the initial CMake setup and partial build):

With the new check enabled:

real    9m0.901s
user    163m31.015s
sys     3m32.260s

without the new check enabled:

real    8m16.128s
user    151m31.455s
sys     3m23.315s

So, the check is also slower here but not as crazy as in GitHub actions.

@sebproell
Copy link
Contributor Author

I hacked my local clang-tidy installation to extract the timings for different checks:

Total                                              7582.16
performance-unnecessary-value-param                1437.7
bugprone-reserved-identifier                       773.906
modernize-type-traits                              725.428
modernize-macro-to-enum                            649.872
modernize-use-nullptr                              465.248
performance-unnecessary-copy-initialization        447.684
performance-move-const-arg                         408.061
modernize-deprecated-ios-base-aliases              379.476
modernize-replace-auto-ptr                         356.463
modernize-use-using                                236.725
modernize-use-bool-literals                        231.525
performance-type-promotion-in-math-fn              191.077
modernize-redundant-void-arg                       180.396
modernize-use-noexcept                             138.601
modernize-use-uncaught-exceptions                  118.016
modernize-use-equals-default                       113.685
readability-qualified-auto                         68.666
modernize-loop-convert                             59.8898
performance-no-int-to-ptr                          56.1777
modernize-replace-random-shuffle                   56.1364
modernize-use-equals-delete                        48.3806
modernize-avoid-bind                               47.1116
modernize-use-emplace                              46.3092
mpi-type-mismatch                                  43.0508
performance-noexcept-swap                          40.6276
performance-avoid-endl                             40.3881
mpi-buffer-deref                                   37.5569
performance-inefficient-algorithm                  35.5613
performance-noexcept-move-constructor              31.2544
modernize-make-shared                              22.6631
modernize-deprecated-headers                       20.1787
modernize-make-unique                              17.9784
cppcoreguidelines-pro-type-static-cast-downcast    14.6678
performance-move-constructor-init                  12.6691
misc-definitions-in-headers                        6.9102
performance-faster-string-find                     6.1621
modernize-shrink-to-fit                            5.4204
performance-inefficient-vector-operation           4.3713
modernize-unary-static-assert                      2.2121
performance-trivially-destructible                 2.0579
performance-noexcept-destructor                    1.4932
performance-for-range-copy                         0.2467
performance-implicit-conversion-in-loop            0.1536

@bangerth
Copy link
Member

So what do we want to do about this? The CI isn't my forte, so others will have to make the call.

@masterleinad
Copy link
Member

So what do we want to do about this? The CI isn't my forte, so others will have to make the call.

I don't think it's worth spending more time on this.

@sebproell
Copy link
Contributor Author

I don't have an idea right now. Closing.

@sebproell sebproell closed this Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants