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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

(I've not had the chance to complete a full review, but these are some thoughts thus far.)



================
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:
----------------
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?


================
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
----------------
Are you intending to fix this -- that sounds rather serious?


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:97
+
+  /// The helper struct used to record one increment occurence, with all the
+  /// details nessesary
----------------
occurence -> occurrence


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:98
+  /// The helper struct used to record one increment occurence, with all the
+  /// details nessesary
+  struct Detail final {
----------------
Missing a full stop.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:99
+  /// details nessesary
+  struct Detail final {
+    const SourceLocation Loc;     /// What caused the increment?
----------------
I don't think the `final` adds value here.


================
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
+
----------------
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).


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:118
+
+      unsigned MsgId;           /// The id of the message to output
+      unsigned short Increment; /// How much of an increment?
----------------
We use `//` instead of `///` unless we're specifically documenting something for doxygen.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:139
+  };
+  // static_assert(sizeof(Detail) <= 8, "it's best to keep the size minimal");
+
----------------
Why is this commented out?


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:150
+
+  /// The grand total Cognitive Complexity of the function
+  unsigned Total = 0;
----------------
Missing full-stop (here and elsewhere, I'll stop commenting on them).


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:191
+  Details.emplace_back(Loc, Nesting, C);
+  const Detail &D(Details.back());
+
----------------
Please use `=` instead of `()` here.


================
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>>
----------------
What's with the comment?


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:215-218
+#define TraverseWithIncreasedNestingLevel(CLASS, Node)                         \
+  ++CurrentNestingLevel;                                                       \
+  ShouldContinue = Base::Traverse##CLASS(Node);                                \
+  --CurrentNestingLevel;
----------------
I don't think this macro adds clarify for its two uses.


Repository:
  rL LLVM

https://reviews.llvm.org/D36836