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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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
----------------
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.


================
Comment at: libcxx/.clang-tidy:50
+# readability-redundant-member-init,
+# readability-simplify-boolean-expr,
----------------
>>! 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/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120925