[PATCH] D36836: [clang-tidy] Implement readability-function-cognitive-complexity check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:76
+  // So either static out-of-line or non-static in-line.
+  const std::array<const StringRef, 4> Msgs = {{
+      // FIXME: these messages somehow trigger an assertion:
----------------
aaron.ballman wrote:
> I don't think this needs to be a member of the class -- it can be a `static const char *[]` at file scope, can it not?
It seemed  fitting to keep it in the class, because it is directly related to the previous `enum`.
But i guess i agree, it is better to move it back out of the class...


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:77
+  const std::array<const StringRef, 4> Msgs = {{
+      // FIXME: these messages somehow trigger an assertion:
+      // Fix conflicts with existing fix! The new replacement overlaps with an
----------------
aaron.ballman wrote:
> Are you intending to fix this -- that sounds rather serious?
I do intend to fix it, as soon as i'm aware of how to fix this.
Specifically, https://reviews.llvm.org/D36836#889350
```
Question:
Is it expected that clang-tidy somehow parses the DiagnosticIDs::Note's as FixIt's, and thus fails with the following assert:
...
```
I.e. what is the proper way to fix this, should i change the message, or change the code not to parse the message as FixIt?


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:99
+  /// details nessesary
+  struct Detail final {
+    const SourceLocation Loc;     /// What caused the increment?
----------------
aaron.ballman wrote:
> I don't think the `final` adds value here.
I don't think it hurts, either?
I *personally* prefer to specify it always, and remove if subclassing is needed.
But if you insist i can certainly drop it


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102
+    const unsigned short Nesting; /// How deeply nested is Loc located?
+    const Criteria C : 3;         /// The criteria of the increment
+
----------------
aaron.ballman wrote:
> Why is this turned into a bit-field? If that is important, it should be of type `unsigned` to prevent differences between compilers (MSVC treats bit-fields differently than GCC and Clang).
The comment right after this member variable should have explained it:
```
/// Criteria C is a bitfield. Even though Criteria is an unsigned char; and
/// only using 3 bits will still result in padding, the fact that it is a
/// bitfield is a reminder that it is important to min(sizeof(Detail))
```
It is already `unsigned`:
```
enum Criteria : unsigned char {
```


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:150
+
+  /// The grand total Cognitive Complexity of the function
+  unsigned Total = 0;
----------------
aaron.ballman wrote:
> Missing full-stop (here and elsewhere, I'll stop commenting on them).
Poured-in some full-stops. I doubt i caught all the places / did not add it in wrong places.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:210
+  /// new sequence, thus it is a stack.
+  std::stack</*********/ Optional<BinaryOperator::Opcode>,
+             SmallVector<Optional<BinaryOperator::Opcode>, 4>>
----------------
aaron.ballman wrote:
> What's with the comment?
An ugly attempt to align the same substring `Optional<BinaryOperator::Opcode>` on both lines.
I can either drop the comment, or do `using OBO = Optional<BinaryOperator::Opcode>;` and use it.


Repository:
  rL LLVM

https://reviews.llvm.org/D36836