lebedev.ri added inline comments.
================
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:
----------------
aaron.ballman wrote:
> 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?
It seemed fitting to keep it in the class, because it is directly related to
the previous `enum`.
But i guess i agree, it is better to move it back out of the class...
================
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
----------------
aaron.ballman wrote:
> Are you intending to fix this -- that sounds rather serious?
I do intend to fix it, as soon as i'm aware of how to fix this.
Specifically, https://reviews.llvm.org/D36836#889350
```
Question:
Is it expected that clang-tidy somehow parses the DiagnosticIDs::Note's as
FixIt's, and thus fails with the following assert:
...
```
I.e. what is the proper way to fix this, should i change the message, or change
the code not to parse the message as FixIt?
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:99
+ /// details nessesary
+ struct Detail final {
+ const SourceLocation Loc; /// What caused the increment?
----------------
aaron.ballman wrote:
> I don't think the `final` adds value here.
I don't think it hurts, either?
I *personally* prefer to specify it always, and remove if subclassing is needed.
But if you insist i can certainly drop it
================
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:
> 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 {
```
================
Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:150
+
+ /// The grand total Cognitive Complexity of the function
+ unsigned Total = 0;
----------------
aaron.ballman wrote:
> Missing full-stop (here and elsewhere, I'll stop commenting on them).
Poured-in some full-stops. I doubt i caught all the places / did not add it in
wrong places.
================
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>>
----------------
aaron.ballman wrote:
> What's with the comment?
An ugly attempt to align the same substring `Optional<BinaryOperator::Opcode>`
on both lines.
I can either drop the comment, or do `using OBO =
Optional<BinaryOperator::Opcode>;` and use it.
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
