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