[libc++] Enable more clang-tidy checks and list potential candidates

[philnik]

Summary

These are some checks that make sense in libc++ IMO. The checks after #TODO: investigate these checks are candidates, but they can't be enabled without some cleanup.

[ldionne]

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.

[philnik]

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.

[• Quuxplusone]

libcxx/.clang-tidy

27–30

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.

50

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/

[philnik]

libcxx/.clang-tidy

27–30

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)

50

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.

[philnik]

Rebased

[ldionne]

Please make sure CI passes.

libcxx/.clang-tidy

 InheritParentConfig: true
-Checks: '-readability-identifier-naming,-llvm-header-guard,-misc-unconventional-assign-operator,-llvm-else-after-return,-llvm-qualified-auto'
+Checks: >
+  bugprone-copy-constructor-init,
+  bugprone-dangling-handle,
+  bugprone-infinite-loop,
+
+  -llvm-else-after-return,
+  -llvm-header-guard,
+  -llvm-qualified-auto,
+
+  -misc-unconventional-assign-operator,
+
+  modernize-redundant-void-arg,
+
+  readability-duplicate-include,
+  readability-function-cognitive-complexity,
+  readability-function-size,
+  -readability-identifier-naming,
+  readability-misplaced-array-index,
+  readability-redundant-control-flow,
+  readability-redundant-function-ptr-dereference,
+  readability-redundant-preprocessor,
+  readability-simplify-subscript-expr,
+  readability-uniqueptr-delete-release,
+
+CheckOptions:
+  - 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
+
+# TODO: investigate these checks
+# bugprone-branch-clone,
+# bugprone-macro-parentheses,
+# cppcoreguidelines-prefer-member-initializer,
+# modernize-loop-convert,
+# modernize-use-bool-literals,
+# modernize-use-default-member-init,
+# modernize-use-equals-default,
+# modernize-use-equals-delete,
+# modernize-use-nullptr,
+# modernize-use-override,
+# portability-restrict-system-includes,
+# readability-function-cognitive-complexity,
+# readability-implicit-bool-conversion,
+# readability-isolate-declaration,
+# readability-redundant-access-specifiers,
+# readability-redundant-declaration,
+# readability-redundant-member-init,
+# readability-simplify-boolean-expr,