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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Looking up to the Visitor, i will do this next.



================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:27
+  // Here are all the possible reasons:
+  enum Criteria : unsigned char {
+    None = 0,
----------------
i think clarifying which language constructs relative to what criteria would help here.

with the document next to me here its clear, but i think we shouldn't expect that. a link/refernce to the page would be nice, too.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:51
+    const SourceLocation Loc; // what caused the increment?
+    const unsigned short Nesting; // how deeply nestedly is Loc located?
+    const Criteria C : 3; // the criteria of the increment
----------------
s/nestedly/nested/

and make the trailing comments doxygen style? might help, but not so important i think.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:52
+    const unsigned short Nesting; // how deeply nestedly is Loc located?
+    const Criteria C : 3; // the criteria of the increment
+
----------------
is this a bitfield? How does it help here? A byte should already be started anyway, so leaving 5 bits empty wouldn't have an impact on the size of `Detail`


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:57
+
+    // To minimize the the sizeof(Detail), we only store the minimal info there.
+    // This function is used to convert from the stored info into the usable
----------------
duplicated 'the'


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:62
+    std::pair<const char *, unsigned short> Process() const {
+      assert(C != Criteria::None && "invalid criteria");
+
----------------
You acces `Critera` always scoped. I think you could declare `Criteria` to be a `enum class`, not just a plain `enum`


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114
+const std::array<const char *const, 4> CognitiveComplexity::Msgs = {{
+    // B1 + B2 + B3
+    "+%0, including nesting penalty of %1, nesting level increased to %2",
+
+    // B1 + B2
+    "+%0, nesting level increased to %2",
+
----------------
could this initialization land in line 45? that would be directly close to the criteria. 


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:116-146
+// Criteria is a bitset, thus a few helpers are needed
+static CognitiveComplexity::Criteria
+operator|(CognitiveComplexity::Criteria lhs,
+          CognitiveComplexity::Criteria rhs) {
+  return static_cast<CognitiveComplexity::Criteria>(
+      static_cast<std::underlying_type<CognitiveComplexity::Criteria>::type>(
+          lhs) |
----------------
`Criteria` could be moved out of the `Detail` struct. That would allow closer definiton of the helper code to the `enum`.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:154
+  Details.emplace_back(Loc, Nesting, C);
+  const Detail &d(Details.back());
+
----------------
s/d/D/ naming convention


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:168
+  // Used to efficiently know the last type of binary sequence operator that
+  // was encoutered. It would make sense for the function call to start the
+  // new sequence, thus it is a stack.
----------------
s/encoutered/encountered/


Repository:
  rL LLVM

https://reviews.llvm.org/D36836