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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
