[libcxx-commits] [PATCH] D120925: [libc++] Enable more clang-tidy checks and list potential candidates

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked an inline comment as done.
philnik added inline comments.


================
Comment at: libcxx/.clang-tidy:27-30
+  - key:   readability-function-cognitive-complexity.Threshold
+    value: 143 # TODO: bring that number down
+  - key:   readability-function-size.LineThreshold
+    value: 194 # TODO: bring that number down
----------------
Quuxplusone wrote:
> I'd prefer not to encode arbitrary "magic numbers" in this file. IIUC, you picked these numbers basically by looking at all the code we've written in libc++ so far, taking the maximum (McCabe complexity|LoC length|whatever), and writing it down right here. But that's basically implying a rule: "We should never write any code that's more (complex|long|whatever) than we have ever written in the past." With all the new stuff in C++20, I don't think such a rule is justified.
I think it's exactly the opposite: we //should// make the code more readable. I'm not saying that we should get these numbers as low as possible at any cost, but I //do// think that these functions should be refactored. It's not that hard to do and can significantly increase readability.
What things in C++20 do you think warrant such complex code?
(The worst offenders are `money_get::__do_get`, `__bracket_expression::__exec` and `__intosort`)


================
Comment at: libcxx/.clang-tidy:50
+# readability-redundant-member-init,
+# readability-simplify-boolean-expr,
----------------
Quuxplusone wrote:
> >>! In D120925#3358053, @philnik wrote:
> >>>! In D120925#3357999, @ldionne wrote:
> >> I am favourable to this patch, however I think we all need to be on the same page that `clang-tidy` is not the absolute truth -- if we write some code that we deem valid or good for some reason, and if `clang-tidy` thinks the code is smelly, I don't want us to start blindly trying to satisfy `clang-tidy`. IOW, this is a tool to help **us** write better code -- it can't make decisions for us.
> > 
> > I'd like to add the invariant that there has to be a comment explaining why clang-tidy is wrong somewhere. If clang-tidy complains it probably looks like a smell to a human reader, or it is a clang-tidy bug in which case we should add a link to the bug report. I'd also like to avoid a plain `// NOLINT` (and variants). Using `// NOLINT(check)` (and variants) makes it clear what clang-tidy warns about.
> 
> As with `//clang-format off`, I'd strongly prefer to avoid `//NOLINT` comments. They add distraction for the human maintainer, and serve no beneficial purpose. 150% what Louis said: We should use computer tools to enforce consistency of libc++'s //own// style and rules; we should not add new rules motivated simply by the fact that there happens to be a convenient computer tool for checking them.
> https://quoteinvestigator.com/2013/04/11/better-light/
I'm not saying we should sprinkle the code with `// NOLINT` comments. I'm saying that if clang-tidy complains we should probably add a comment, because the code is in some way unusual. The other option is that it's a clang-tidy bug, in which case we should report it and remove the `// NOLINT` comment as soon as the bug has been fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120925/new/

https://reviews.llvm.org/D120925