[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:102
+    const unsigned short Nesting; /// How deeply nested is Loc located?
+    const Criteria C : 3;         /// The criteria of the increment
+
----------------
aaron.ballman wrote:
> lebedev.ri wrote:
> > 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 {
> > ```
> No, it's underlying type is `unsigned char`, but the type is `Criteria`. I meant the field itself needs to be `unsigned`. However, since that means you have to do more type casting, I think making the type `uint8_t` instead of a bit-field would be an improvement.
Better?


================
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?
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > We use `//` instead of `///` unless we're specifically documenting something for doxygen.
> This change is marked as done but wasn't applied consistently. For instance, see lines 130-131 where it switches between uses. I would use `//` consistently everywhere unless documenting a public interface in a header.
OK.


================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:495
+  Finder->addMatcher(
+      functionDecl(unless(anyOf(isInstantiated(), isImplicit()))).bind("func"),
+      this);
----------------
aaron.ballman wrote:
> You only need to match function *definitions* (not declarations), correct? It might be useful to specify that in the matcher so that it matches less code.
Good idea!


Repository:
  rL LLVM

https://reviews.llvm.org/D36836