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

lebedev.ri added a comment. In D36836#2310170 , @lebedev.ri wrote: > In D36836#2310169 , @lebedev.ri > wrote: > >> In D36836#2310168 , @RKSimon wrote: >> >>> @lebedev.ri

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

lebedev.ri added a comment. In D36836#2310169 , @lebedev.ri wrote: > In D36836#2310168 , @RKSimon wrote: > >> @lebedev.ri This is failing on the windows arm build bots - please can you >> take a look? >> >>

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

lebedev.ri added a comment. In D36836#2310168 , @RKSimon wrote: > @lebedev.ri This is failing on the windows arm build bots - please can you > take a look? > > http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/1482 >

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

RKSimon added a comment. @lebedev.ri This is failing on the windows arm build bots - please can you take a look? http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/1482 http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/3285 Repository: rG LLVM Github Monorepo

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGace644030e67: [clang-tidy] Implement

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

lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. @aaron.ballman @JonasToth thank you for the review! @tonic @lattner thank you for helping resolve the misunderstading :) > ! In D36836#2309432 , @aaron.ballman > wrote: > LGTM,

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

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, thank you for your patience with the process. Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:381 +// FIXME: "each method in a recursion cycle" Increment

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

lebedev.ri marked 4 inline comments as done. lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:381 +// FIXME: "each method in a recursion cycle" Increment is not implemented. +case

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:367-368 + bool TraverseStmt(Stmt *Node) { +if (!Node) + return Base::TraverseStmt(Node); + lebedev.ri wrote: > aaron.ballman

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

lebedev.ri updated this revision to Diff 295904. lebedev.ri added a comment. Maybe fine tune comments - we count block decls, not block statements/expressions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D36836/new/

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

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:367-368 + bool TraverseStmt(Stmt *Node) { +if (!Node) + return Base::TraverseStmt(Node); + aaron.ballman wrote: > If there's

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

lebedev.ri updated this revision to Diff 295902. lebedev.ri marked 10 inline comments as done. lebedev.ri added a comment. In D36836#2309243 , @aaron.ballman wrote: > In D36836#2308981 , @lattner wrote: > >> Hi

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

aaron.ballman added a comment. In D36836#2308981 , @lattner wrote: > Hi all, > > The LLVM foundation board discussed this offline -- it looks like the > contributor is a bit confused about the LLVM project policies towards > copyright, license, patent

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:93-96 +// Criteria only uses three bits, so uint8_t is used as an underlying type. +// We could make C a bitfield, but then we would not save

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

lebedev.ri updated this revision to Diff 295879. lebedev.ri retitled this revision from "[clang-tidy] Implement sonarsource-function-cognitive-complexity check" to "[clang-tidy] Implement readability-function-cognitive-complexity check". lebedev.ri added a comment. @aaron.ballman @alexfh it is

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

lebedev.ri added a comment. Ping. In https://reviews.llvm.org/D36836#889375, @aaron.ballman wrote: > Adding @dberlin for licensing discussion questions. @dberlin ping? I'm wondering if you had the chance to look at this? :) Repository: rL LLVM https://reviews.llvm.org/D36836

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

lebedev.ri updated this revision to Diff 122053. lebedev.ri added a comment. Ping. In https://reviews.llvm.org/D36836#889375, @aaron.ballman wrote: > Adding @dberlin for licensing discussion questions. @dberlin ping? I'm wondering if you had the chance to look at this? :) Repository: rL

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

lebedev.ri updated this revision to Diff 120890. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. Ping. Rebased. Silenced (via the `-w` flag in the `RUN` line) the `Fix conflicts with existing fix!` assertion since this differential does not actually cause it, but the

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

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:499 + functionDecl( + allOf(isDefinition(), unless(anyOf(isInstantiated(), isImplicit() + .bind("func"), aaron.ballman wrote: >

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

lebedev.ri updated this revision to Diff 120407. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Rebased. Tighten matchers. Add an assert that the function we matched does have the body (the check analyses the function bodies, so if there is no body, there is nothing to

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

aaron.ballman 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 +

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

lebedev.ri updated this revision to Diff 119766. lebedev.ri marked 28 inline comments as done. lebedev.ri added a comment. Rebased. Addressed @aaron.ballman review notes (mainly stylistic) Repository: rL LLVM https://reviews.llvm.org/D36836 Files: LICENSE.TXT

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

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 +

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

aaron.ballman added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:77 + const std::array Msgs = {{ + // FIXME: these messages somehow trigger an assertion: + // Fix conflicts with existing fix! The new replacement overlaps

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

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 Msgs = {{ + // FIXME: these messages somehow trigger an assertion:

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

lebedev.ri updated this revision to Diff 119091. lebedev.ri marked 9 inline comments as done and an inline comment as not done. lebedev.ri added a comment. @aaron.ballman, thank you for the review! Rebased, addressed most of review notes. Repository: rL LLVM https://reviews.llvm.org/D36836

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

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

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#889375, @aaron.ballman wrote: > Adding @dberlin for licensing discussion questions. Ping. Yes, i agree that what i have added is not a directory, and not a proper license. That is more of a template to hopefully stat moving

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

aaron.ballman added a reviewer: dberlin. aaron.ballman added a subscriber: dberlin. aaron.ballman added a comment. Adding @dberlin for licensing discussion questions. Comment at: LICENSE.TXT:64 clang-tidy clang-tidy/hicpp +clang-tidy

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

lebedev.ri updated this revision to Diff 117824. lebedev.ri added a comment. Ping. - Rebased - Added the legal status info into `LICENSE.txt`, referencing `clang-tidy/readability/FunctionCognitiveComplexityCheck.LICENSE.txt`, as suggested by @aaron.ballman Question: Is it expected that

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

aaron.ballman added a comment. In https://reviews.llvm.org/D36836#883214, @lebedev.ri wrote: > @alexfh please specify, is that enough or not? I think it's sufficient, but we should put that information somewhere pertinent rather than just the review. In the past, we've put something into

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

lebedev.ri added a comment. Ping. In https://reviews.llvm.org/D36836#877642, @lebedev.ri wrote: > In https://reviews.llvm.org/D36836#877636, @alexfh wrote: > > > > The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource > > > specification version 1.2 (19 April 2017), > > > > Err,

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

JonasToth added a comment. Hi Roman, there are still some comments marked as `not done` from my review. Maybe other reviewers wait for there 'resolution', but from my side there are resolved. One more attempt to get the review rolling. Repository: rL LLVM https://reviews.llvm.org/D36836

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#877636, @alexfh wrote: > > The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource > > specification version 1.2 (19 April 2017), > > Err, "I did ask them, and received an answer that it is it can be implemented > in

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

alexfh added a comment. > The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource > specification version 1.2 (19 April 2017), Err, "I did ask them, and received an answer that it is it can be implemented in clang-tidy" might not be enough. Repository: rL LLVM

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#873246, @JonasToth wrote: > For my part the current state is ok. @JonasToth thank you for the review! > but @alexfh and @aaron.ballman should do their review before committing. +1 :) Now what one full review is done, it may be

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

JonasToth added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114 +const std::array CognitiveComplexity::Msgs = {{ +// B1 + B2 + B3 +"+%0, including nesting penalty of %1, nesting level increased to %2", + +// B1 + B2 +

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

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114 +const std::array CognitiveComplexity::Msgs = {{ +// B1 + B2 + B3 +"+%0, including nesting penalty of %1, nesting level increased to %2", + +// B1 + B2 +

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

JonasToth added a comment. For my part the current state is ok. @alexfh and @aaron.ballman but should do their review before committing. I would be interested in a exampleoutput for any real project. Comment at:

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

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:102-114 +const std::array CognitiveComplexity::Msgs = {{ +// B1 + B2 + B3 +"+%0, including nesting penalty of %1, nesting level increased to %2", + +// B1 + B2 +

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

lebedev.ri updated this revision to Diff 115547. lebedev.ri marked 13 inline comments as done. lebedev.ri added a comment. Address even more @JonasToth's review notes. - Fixed docs - Validated the implementation (the testcase) against the "reference" implementation - Fixed discrepancies in the

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

JonasToth added a comment. I see, forgot that. The doc were the interesting part for me, I trust that you test the code correctlty :) Repository: rL LLVM https://reviews.llvm.org/D36836 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#873032, @JonasToth wrote: > I built the patch locally, here is my output: > > The Unittest failed for me: `Neither CHECK-FIXES nor CHECK-MESSAGES found in > the input` > > Script call: > `/usr/bin/python2.7 >

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

JonasToth added a comment. I built the patch locally, here is my output: The Unittest failed for me: `Neither CHECK-FIXES nor CHECK-MESSAGES found in the input` Script call: `/usr/bin/python2.7 /home/jonas/opt/llvm/tools/clang/tools/extra/test/../test/clang-tidy/check_clang_tidy.py

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

lebedev.ri added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:62 +std::pair Process() const { + assert(C != Criteria::None && "invalid criteria"); + JonasToth wrote: > You acces `Critera` always scoped. I

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

lebedev.ri updated this revision to Diff 115482. lebedev.ri marked 13 inline comments as done. lebedev.ri added a comment. Address most of the @JonasToth's review notes. Repository: rL LLVM https://reviews.llvm.org/D36836 Files: clang-tidy/readability/CMakeLists.txt

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

JonasToth added inline comments. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:27 + // Here are all the possible reasons: + enum Criteria : unsigned char { +None = 0, JonasToth wrote: > i think clarifying which language constructs

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

JonasToth added a comment. I did read through it now. In general, the code is sound with my understanding of the complexity metric and there is a almost one to one wording to the document, which is nice. Since we chatted in IRC directly, i would give a short summary to avoid forgetting what we

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

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

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

aaron.ballman added reviewers: sbenza, JonasToth. aaron.ballman added a comment. Adding a few reviewers to hopefully help Roman get some feedback. Repository: rL LLVM https://reviews.llvm.org/D36836 ___ cfe-commits mailing list

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

lebedev.ri updated this revision to Diff 114549. lebedev.ri added a comment. @alexfh et al: friendly ping :) Repository: rL LLVM https://reviews.llvm.org/D36836 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

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

lebedev.ri added a comment. I understand that everyone is likely busy with 5.0, but still, give me at least some feedback, please :) ping Repository: rL LLVM https://reviews.llvm.org/D36836 ___ cfe-commits mailing list

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

lebedev.ri updated this revision to Diff 113230. lebedev.ri added a comment. Rebased the correct branch this time... When splitting off https://reviews.llvm.org/D36892, and addressing the other review note in this Revision, i slightly messed up the local branches, so the wrong code got rebased

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

lebedev.ri updated this revision to Diff 113228. lebedev.ri added a comment. Rebased. Repository: rL LLVM https://reviews.llvm.org/D36836 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

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

lebedev.ri added a comment. Ping 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

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

Since i initially forgot to include cfe-commits as a subscriber into the review, the first mail was not sent to the list. So maybe, for history and clarity, i should forward the mail to the list: (i'm keeping the reviewers as CC since phabricator, unlike github, does not add mail-based reviews as

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

lebedev.ri added a comment. In https://reviews.llvm.org/D36836#845417, @JonasToth wrote: > Did I run it over clang or athletic like that? I would be interested in an > overall output. No, not yet. But if it is to be enabled for LLVM repos by default, then the `Threshold` will most

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

lebedev.ri updated this revision to Diff 111731. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. Address review notes: - `constexpr const` -> `constexpr` - moved `test/clang-tidy/check_clang_tidy.py` changes into https://reviews.llvm.org/D36892 Repository: rL LLVM

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

JonasToth added a comment. Did I run it over clang or athletic like that? I would be interested in an overall output. I added my thoughts. This check seems very valuable. Comment at: clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:23 + // Limit of 25 is the