This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement readability-function-cognitive-complexity check
ClosedPublic

Authored by lebedev.ri on Aug 17 2017, 8:57 AM.

Details

Summary

Currently, there is basically just one clang-tidy check to impose some sanity limits on functions - clang-tidy-readability-function-size.
It is nice, allows to limit line count, total number of statements, number of branches, number of function parameters (not counting implicit this), nesting level.
However, those are simple generic metrics. It is still trivially possible to write a function, which does not violate any of these metrics, yet is still rather unreadable.

Thus, some additional, slightly more complicated metric is needed.
There is a well-known Cyclomatic complexity, but certainly has its downsides.
And there is a COGNITIVE COMPLEXITY by SonarSource, which is available for opensource on https://sonarcloud.io/.
I did ask them, and received an answer that it is it can be implemented in clang-tidy.

There is a number of official open-source LGPL-3 implementations already:

There are other open-source LGPL-3 implementations already:

There are other 3rd party implementations:

This check checks function Cognitive Complexity metric, and flags the functions with Cognitive Complexity exceeding the configured limit.
The default limit is 25, same as in 'upstream'.

The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource specification version 1.2 (19 April 2017), with two notable exceptions:

  • preprocessor conditionals (#ifdef, #if, #elif, #else, #endif) are not accounted for. Could be done. Currently, upstream does not account for them either.
  • each method in a recursion cycle is not accounted for. It can't be fully implemented, because cross-translational-unit analysis would be needed, which is not possible in clang-tidy. Thus, at least right now, i completely avoided implementing it.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lebedev.ri added inline comments.Sep 16 2017, 3:25 PM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103–115

Actually, found a combination that works, done.
FIXME: so should const std::array<const StringRef, 4> CognitiveComplexity::Msgs be static and initialized out-of-line, or not static, but initialized in-line?

207

Modified, but without templates.

229–235

Actually, it turned out to be much simper now that i cross-referenced with the output of the "reference implementation"

docs/ReleaseNotes.rst
145

Reworded once more

docs/clang-tidy/checks/readability-function-cognitive-complexity.rst
11

did you test them?

No, actually.
Thanks for the hint on how to build them, now fixed.

JonasToth added a comment.EditedSep 17 2017, 1:20 AM

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

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103–115

Since it is already in an anonymous namespace, static would be duplicated, wouldn't it? I like it now!

docs/ReleaseNotes.rst
145

Sounds good. :)

lebedev.ri added inline comments.Sep 17 2017, 3:30 AM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103–115

Msgs is in a struct CognitiveComplexity, which is in anonymous namespace

JonasToth added inline comments.Sep 17 2017, 3:34 AM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103–115

Ups. But that solution wouldn't be that bad in term of structure size, would it? The messages themself should land in static program storage, so only 4 pointers would be saved. I would leave it as is, maybe make it constexpr.

lebedev.ri added a comment.EditedSep 17 2017, 4:02 AM

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 pass is done, it may be easier to start for the other reviewers..

I would be interested in a exampleoutput for any real project.

TBD

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103–115

But that solution wouldn't be that bad in term of structure size, would it?

I *think* so

constexpr

Comment from before const std::array<const StringRef, 4> Msgs = {{

// Yes, this member variable would be better off being `static`. But then
// either `StringRef` does not have `constexpr` constructor (because `strlen` is not
// `constexpr`), which is needed for `static constexpr` variable; or if `char*` is
// used, there is a linking problem (Msgs is missing).
// So either static out-of-line or non-static in-line.
alexfh edited edge metadata.Sep 21 2017, 8:22 AM

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.

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.

Hello Roman,

It would not, and I would be happy to be notified once done.

Thanks

Olivier

> On Fri, Jul 28, 2017 at 6:17 PM Roman Lebedev <lebedev.ri@gmail.com> wrote:
> Hi.
> 
> I would like to know, what is the legal status of
> https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
> 
> If i (or someone else) is to implement the Specification described in
> that paper,
> which will produce the same results as the Cognitive Complexity in SonarSource
> as part of some other static analyzer, for example as a LLVM's clang-tidy check,
> would that be illegal thing to do?
> 
> Roman.
-- 
Olivier Gaudin | SonarSource
CEO & Co-Founder
+41 (0)22 510 2424
http://sonarsource.com

Might not be enough?

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.

Ping.

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.

Hello Roman,

It would not, and I would be happy to be notified once done.

Thanks

Olivier

> On Fri, Jul 28, 2017 at 6:17 PM Roman Lebedev <lebedev.ri@gmail.com> wrote:
> Hi.
> 
> I would like to know, what is the legal status of
> https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
> 
> If i (or someone else) is to implement the Specification described in
> that paper,
> which will produce the same results as the Cognitive Complexity in SonarSource
> as part of some other static analyzer, for example as a LLVM's clang-tidy check,
> would that be illegal thing to do?
> 
> Roman.
-- 
Olivier Gaudin | SonarSource
CEO & Co-Founder
+41 (0)22 510 2424
http://sonarsource.com

Might not be enough?

@alexfh please specify, is that enough or not?

@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 LICENSE.TXT to identify that we're in the clear, and that might be appropriate here as well. What do you think, @alexfh?

jbcoe added a subscriber: jbcoe.Sep 28 2017, 3:05 PM
lebedev.ri updated this revision to Diff 117824.Oct 5 2017, 8:08 AM

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 clang-tidy somehow parses the DiagnosticIDs::Note's as FixIt's, and thus fails with the following assert:

Running ['clang-tidy', '/build/llvm-build-Clang-release/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp.cpp', '-fix', '--checks=-*,readability-function-cognitive-complexity', '-config={CheckOptions: [{key: readability-function-cognitive-complexity.Threshold, value: 0}]}', '--', '-std=c++11', '-nostdinc++']...
clang-tidy failed:
Fix conflicts with existing fix! The new replacement overlaps with an existing replacement.
New replacement: /build/llvm-build-Clang-release/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp.cpp: 3484:+5:""
Existing replacement: /build/llvm-build-Clang-release/tools/clang/tools/extra/test/clang-tidy/Output/readability-function-cognitive-complexity.cpp.tmp.cpp: 3485:+2:"&"
clang-tidy: /build/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:86: virtual void (anonymous namespace)::ClangTidyDiagnosticRenderer::emitCodeContext(clang::FullSourceLoc, DiagnosticsEngine::Level, SmallVectorImpl<clang::CharSourceRange> &, ArrayRef<clang::FixItHint>): Assertion `false && "Fix conflicts with existing fix!"' failed.
aaron.ballman added a subscriber: dberlin.

Adding @dberlin for licensing discussion questions.

LICENSE.TXT
64

This should point to a directory instead of an individual file.

clang-tidy/readability/FunctionCognitiveComplexityCheck.LICENSE.txt
3

I think that this file should be a general LICENSE.TXT instead of a tied to a specific file name. The license is for the algorithm, not for the file name.

14

This isn't actually a license, however. I think we need to ask sonarsource either for a license statement they're comfortable with, or approach them with one in hand that they can approve. However, @dberlin might have more input here.

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 things in the right direction.

To clarify, my problem with this Differential is that i'we yet to see any opinion on
the proposed check from the people who will actually have the final say whether
it will be accepted or not. It will be *really* sad if we go through all this trouble
with the legal matters, and then it will be NACK'ed...

(I've not had the chance to complete a full review, but these are some thoughts thus far.)

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
77

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?

78

Are you intending to fix this -- that sounds rather serious?

98

occurence -> occurrence

99

Missing a full stop.

100

I don't think the final adds value here.

103

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).

119

We use // instead of /// unless we're specifically documenting something for doxygen.

140

Why is this commented out?

151

Missing full-stop (here and elsewhere, I'll stop commenting on them).

192

Please use = instead of () here.

211

What's with the comment?

216–219

I don't think this macro adds clarify for its two uses.

lebedev.ri marked 9 inline comments as done and an inline comment as not done.

@aaron.ballman, thank you for the review!

Rebased, addressed most of review notes.

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
77

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...

78

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?

100

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

103

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 {
151

Poured-in some full-stops. I doubt i caught all the places / did not add it in wrong places.

211

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.

aaron.ballman added inline comments.Oct 21 2017, 6:29 AM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
78

This is one for @alexfh to answer, I think. My gut says that the code should not be parsing notes as fixits (as a separate commit).

100

That's not the typical coding style for the project, so I would prefer to leave it off. It's reasonable to use when there are vtables involved, however.

103

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.

112

This static_assert should come with some further comments explaining that size being small is good, but not critical for correctness.

119

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.

213

Declare with its initialization below.

221

Declare with its initialization below.

251

Missing a closing paren in the comment.

276–277

Would be better to just get the AST node directly instead of isa<>() and dyn_cast<>().

if (const auto *E = dyn_cast<IfStmt>(Node->getElse()))

318

encounter function call -> encounter a function call

326

typo: nonexistent

333

if new operator -> if the new operator

334

previous-one -> previous one

339

creates new stack -> creates a new stack

348

Can drop the const.

371

This FIXME doesn't describe what needs to be fixed very well, you might want to expound (or fix it).

385

regular -> Regular

392

else are -> else are (remove extra space)

393

nested -> Nested

412

extra space between else and are

434

if -> If

445

The second non-standard parameter -> The parameter

449

Explaination -> Explanation

496

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.

514–515

I would reword this to: Output all the basic increments of complexity.

521

Increase on the -> Increase, on the

docs/clang-tidy/checks/readability-function-cognitive-complexity.rst
9

as per -> as per the

86

And this -> This

142

I would drop Could be done.

lebedev.ri marked 28 inline comments as done.

Rebased.
Addressed @aaron.ballman review notes (mainly stylistic)

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103

Better?

119

OK.

496

Good idea!

aaron.ballman added inline comments.Oct 25 2017, 2:11 PM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
103

Much, thank you!

500

Since we're restricting this, might as well add deleted and defaulted functions to the list of unless items (as I think those are still definitions).

lebedev.ri marked 3 inline comments as done.

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 do.)

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
500

Good idea!
It seems only the deleted were still matched, not defaulted.

lebedev.ri marked 2 inline comments as done.

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 testcase just happens to trigger it...
Reported it as PR35136 with minimal reproducer.

Adding @dberlin for licensing discussion questions.

@dberlin ping? I'm wondering if you had the chance to look at this? :)

lebedev.ri updated this revision to Diff 122053.Nov 8 2017, 3:48 AM

Ping.

Adding @dberlin for licensing discussion questions.

@dberlin ping? I'm wondering if you had the chance to look at this? :)

Ping.

Adding @dberlin for licensing discussion questions.

@dberlin ping? I'm wondering if you had the chance to look at this? :)

lebedev.ri marked an inline comment as done.
lebedev.ri retitled this revision from [clang-tidy] Implement readability-function-cognitive-complexity check to [clang-tidy] Implement sonarsource-function-cognitive-complexity check.
  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

@chandlerc Hi!
@aaron.ballman has suggested for me to try to talk to you about this.
Is there some precedent for the licensing 'issue' at hand? Do you have any opinion?
@dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

If there are no further ideas, i'll try to contact sonarsource.

lebedev.ri changed the repository for this revision from rL LLVM to rCTE Clang Tools Extra.

Rebased.

  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

@chandlerc Hi!
@aaron.ballman has suggested for me to try to talk to you about this.
Is there some precedent for the licensing 'issue' at hand? Do you have any opinion?
@dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @llvm.org email address if useful.

  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

@chandlerc Hi!
@aaron.ballman has suggested for me to try to talk to you about this.
Is there some precedent for the licensing 'issue' at hand? Do you have any opinion?
@dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @llvm.org email address if useful.

Mailed and heard back (yay!).
They don't want to take active stance/first steps in this,
but they also did *not* retract that initial 'agreement' i quoted earlier (i did ask about that explicitly.).
So nothing is changed so far, this will have to wait until someone can volunteer their precious lawyer time.
(Can show the mail exchange if needed.)

alexfh requested changes to this revision.Mar 14 2018, 8:31 AM

Removing from my dashboard until the licensing stuff gets sorted out.

This revision now requires changes to proceed.Mar 14 2018, 8:31 AM
lebedev.ri updated this revision to Diff 154491.Jul 7 2018, 6:32 AM

Rebased, just to control bitrot, no changes.

Anything new here? Is there a LLVM foundation lawyer or something like that we can ask?

alexfh requested changes to this revision.Oct 1 2018, 6:19 AM

Anything new here? Is there a LLVM foundation lawyer or something like that we can ask?

I didn't notice any substantial changes w.r.t. license since Chandler's comment on Feb. 28th.

This revision now requires changes to proceed.Oct 1 2018, 6:19 AM
  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

@chandlerc Hi!
@aaron.ballman has suggested for me to try to talk to you about this.
Is there some precedent for the licensing 'issue' at hand? Do you have any opinion?
@dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts

Hi @chandlerc !
I was just wondering what percentage of "finish" was meant here, and where at it's now?

and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @ llvm.org email address if useful.

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2019, 2:58 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
JonasToth resigned from this revision.Jan 4 2020, 2:39 PM
mgehre removed a subscriber: mgehre.Jan 4 2020, 3:29 PM
  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

@chandlerc Hi!
@aaron.ballman has suggested for me to try to talk to you about this.
Is there some precedent for the licensing 'issue' at hand? Do you have any opinion?
@dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts

Hi @chandlerc !
I was just wondering what percentage of "finish" was meant here, and where at it's now?

And how far along is LLVM now, one year later? :)

and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @ llvm.org email address if useful.

lebedev.ri edited the summary of this revision. (Show Details)

Rebased.

There is a number of official open-source LGPL-3 implementations already:

There are other open-source LGPL-3 implementations already:

There are other 3rd party implementations:

Quite honestly, i do not understand how did the license question arose.
Would have it been fine if i based this on the open-source-licensed code?
Would have it not been? Would same license question be raised?
Somehow i don't think it would have been.

Is this really just about Copyright SonarSource S.A., 2018, Switzerland. All content is copyright protected. in https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
But that is only about the document, not the algorithm.
But even if we enternain the idea that all of the implementations must bow to that license,
then surely this is not the first code in LLVM that is implicitly/explicitly based on copyrighted doc.

This is rather frustrating.

Rebased.

There is a number of official open-source LGPL-3 implementations already:

There are other open-source LGPL-3 implementations already:

There are other 3rd party implementations:

Quite honestly, i do not understand how did the license question arose.
Would have it been fine if i based this on the open-source-licensed code?
Would have it not been? Would same license question be raised?
Somehow i don't think it would have been.

Is this really just about Copyright SonarSource S.A., 2018, Switzerland. All content is copyright protected. in https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
But that is only about the document, not the algorithm.
But even if we enternain the idea that all of the implementations must bow to that license,
then surely this is not the first code in LLVM that is implicitly/explicitly based on copyrighted doc.

This is rather frustrating.

Given there are open source implementations, this is in my eyes completly acceptable to implement this in llvm.
The document is copyrighted, true. The algorithm itself is not, in european law it is even very hard to patent algorithms, software in general is not patentable.
The code is not copy&pasted, so there are no infringements on anything.

Rebased.

There is a number of official open-source LGPL-3 implementations already:

There are other open-source LGPL-3 implementations already:

There are other 3rd party implementations:

Quite honestly, i do not understand how did the license question arose.

It arose in a comment that I can't seem to get phab to show me the context for (which is a bit strange, I don't think I've run into that before): https://reviews.llvm.org/D36836#877636 Perhaps part of this was carrying discussion over from the IRC channel?

Would have it been fine if i based this on the open-source-licensed code?

I believe that would require legal analysis to answer.

Would have it not been? Would same license question be raised?

Likewise here (I suspect the answer would depend on what the license of the open source code is).

Somehow i don't think it would have been.

I don't wish to speculate about legal licensing issues on the mailing lists.

Is this really just about Copyright SonarSource S.A., 2018, Switzerland. All content is copyright protected. in https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
But that is only about the document, not the algorithm.
But even if we enternain the idea that all of the implementations must bow to that license,
then surely this is not the first code in LLVM that is implicitly/explicitly based on copyrighted doc.

This is rather frustrating.

I am sorry and I agree that it's frustrating. As far as I know, this captures the current state of affairs: https://reviews.llvm.org/D36836#1031600 and basically we're waiting for help from the foundation to clear the last hurdle.

Rebased.

There is a number of official open-source LGPL-3 implementations already:

There are other open-source LGPL-3 implementations already:

There are other 3rd party implementations:

Quite honestly, i do not understand how did the license question arose.

It arose in a comment that I can't seem to get phab to show me the context for (which is a bit strange, I don't think I've run into that before): https://reviews.llvm.org/D36836#877636 Perhaps part of this was carrying discussion over from the IRC channel?

Would have it been fine if i based this on the open-source-licensed code?

I believe that would require legal analysis to answer.

Would have it not been? Would same license question be raised?

Likewise here (I suspect the answer would depend on what the license of the open source code is).

Somehow i don't think it would have been.

I don't wish to speculate about legal licensing issues on the mailing lists.

Is this really just about Copyright SonarSource S.A., 2018, Switzerland. All content is copyright protected. in https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
But that is only about the document, not the algorithm.
But even if we enternain the idea that all of the implementations must bow to that license,
then surely this is not the first code in LLVM that is implicitly/explicitly based on copyrighted doc.

This is rather frustrating.

I am sorry and I agree that it's frustrating.

As far as I know, this captures the current state of affairs: https://reviews.llvm.org/D36836#1031600

As far as I know, yes. Some further back&forth reiterated:

@Roman so you know, none of the non-SonarSource implementations have an official license from us.
We put the spec out in the world and we're happy when someone uses it. And that's all.
I appreciate how frustrated you must be with your implementation caught between a proverbial rock and a hard place.
Unfortunately, we (the company) just aren't willing to do the paperwork.
(G. Ann Campbell)

and basically we're waiting for help from the foundation to clear the last hurdle.

Is foundation even aware of this controversy/situation?
https://reviews.llvm.org/D36836#1021863, which is the last response i got, was 2.5 years ago.
For all we/i know this has gone off their radar.
I understand that it is fully possible that they simply haven't gotten around to it,
but i think it would be important to check that it isn't the case of lost mail.

aaron.ballman edited reviewers, added: tonic, asl, hfinkel; removed: dberlin.Sep 24 2020, 8:29 AM

Rebased.

There is a number of official open-source LGPL-3 implementations already:

There are other open-source LGPL-3 implementations already:

There are other 3rd party implementations:

Quite honestly, i do not understand how did the license question arose.

It arose in a comment that I can't seem to get phab to show me the context for (which is a bit strange, I don't think I've run into that before): https://reviews.llvm.org/D36836#877636 Perhaps part of this was carrying discussion over from the IRC channel?

Would have it been fine if i based this on the open-source-licensed code?

I believe that would require legal analysis to answer.

Would have it not been? Would same license question be raised?

Likewise here (I suspect the answer would depend on what the license of the open source code is).

Somehow i don't think it would have been.

I don't wish to speculate about legal licensing issues on the mailing lists.

Is this really just about Copyright SonarSource S.A., 2018, Switzerland. All content is copyright protected. in https://www.sonarsource.com/docs/CognitiveComplexity.pdf ?
But that is only about the document, not the algorithm.
But even if we enternain the idea that all of the implementations must bow to that license,
then surely this is not the first code in LLVM that is implicitly/explicitly based on copyrighted doc.

This is rather frustrating.

I am sorry and I agree that it's frustrating.

As far as I know, this captures the current state of affairs: https://reviews.llvm.org/D36836#1031600

As far as I know, yes. Some further back&forth reiterated:

@Roman so you know, none of the non-SonarSource implementations have an official license from us.
We put the spec out in the world and we're happy when someone uses it. And that's all.
I appreciate how frustrated you must be with your implementation caught between a proverbial rock and a hard place.
Unfortunately, we (the company) just aren't willing to do the paperwork.
(G. Ann Campbell)

Personally, I'm hopeful that this is sufficient to get us unstuck, but I'm not willing to make the determination simply because of the typical community view on making licensing decisions.

and basically we're waiting for help from the foundation to clear the last hurdle.

Is foundation even aware of this controversy/situation?
https://reviews.llvm.org/D36836#1021863, which is the last response i got, was 2.5 years ago.
For all we/i know this has gone off their radar.
I understand that it is fully possible that they simply haven't gotten around to it,
but i think it would be important to check that it isn't the case of lost mail.

I totally agree that we should double-check as it feels like this fell off the radar and I appreciate you pinging the review. @chandlerc is no longer on the foundation board and it looks like Danny is no longer on phabricator, so I've added some current LLVM foundation board members as reviewers to see if any of them can help us make progress.

tonic added a comment.Sep 24 2020, 8:41 AM

Licensing questions are best sent to the board@llvm.org mailing address. Can you reach out there?

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 stuff etc. I really appreciate the dilligence here to capture the ideas that went into this code.

My understanding is that the code and documentation are not derived from anyone else's code or documentation -- they are fresh implementations based on a paper written in english prose. If that is the case, please remove the LICENSE.TXT file -- it isn't necessary and it is confusing for people. I would also recommend replacing all the references to sonar source with a functional description of what this patch is doing - we aren't talking about adding a feature because of sonar source, my understanding is that this is adding a cyclomatic complexity checker. If and when the feature evolves over time, it would be misleading for it to be called sonar source.

Please note that I didn't do a full code review here, others should do that.

-Chris

lebedev.ri retitled this revision from [clang-tidy] Implement sonarsource-function-cognitive-complexity check to [clang-tidy] Implement readability-function-cognitive-complexity check.

@aaron.ballman @alexfh it is my understanding that i'm to proceed with commit. No explicit acknowledgement from your side is needed.

Hi all,

The LLVM foundation board discussed this offline

Yay, thank you!

it looks like the contributor is a bit confused about the LLVM project policies towards copyright, license, patent stuff etc.

...

I really appreciate the dilligence here to capture the ideas that went into this code.

My understanding is that the code and documentation are not derived from anyone else's code or documentation -- they are fresh implementations based on a paper written in english prose. If that is the case,

That is the case, yes. This is based on the spec.

please remove the LICENSE.TXT file -- it isn't necessary and it is confusing for people. I would also recommend replacing all the references to sonar source with a functional description of what this patch is doing - we aren't talking about adding a feature because of sonar source,

Done, as much as possible. I'd still prefer to maintain a reference to the spec on which it is based.
Note that it was done because that is what was requested previously by reviewers.

my understanding is that this is adding a cyclomatic complexity checker.

Well, yes and no. Note that this is *NOT* a well-known cyclomatic complexity metric, but a novel cognitive complexity metric.

If and when the feature evolves over time, it would be misleading for it to be called sonar source.

Yes.

Please note that I didn't do a full code review here, others should do that.

All good on that front, this has been quite extensively reviewed years ago :)

-Chris

Roman

aaron.ballman added inline comments.Oct 2 2020, 12:11 PM
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
93–96 ↗(On Diff #295879)

I'm not certain this comment adds a whole lot.

154 ↗(On Diff #295879)

outputed -> output

155 ↗(On Diff #295879)

Criteria's -> Criteria

222 ↗(On Diff #295879)

of binary -> of the binary

367–368 ↗(On Diff #295879)

If there's not a node, do we really need to traverse it?

381 ↗(On Diff #295879)

BinaryConditionalOperatorClass as well (for all the places we're dealing with ConditionalOperatorClass)?

388 ↗(On Diff #295879)

Should IndirectGotoStmtClass be handled the same way?

410 ↗(On Diff #295879)

BlockExprClass as well?

Should GNU statement expressions also be treated like a lambda or block?

428 ↗(On Diff #295879)

SEHExceptStmtClass as well?

473 ↗(On Diff #295879)

What about other special member functions like overloaded operators and whatnot? How about block declarations?

Namespaces is another one I was curious about but suspect may not want to be handled here.

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h
16–20 ↗(On Diff #295879)

This shouldn't be necessary, right?

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 stuff etc. I really appreciate the dilligence here to capture the ideas that went into this code.

My understanding is that the code and documentation are not derived from anyone else's code or documentation -- they are fresh implementations based on a paper written in english prose. If that is the case, please remove the LICENSE.TXT file -- it isn't necessary and it is confusing for people. I would also recommend replacing all the references to sonar source with a functional description of what this patch is doing - we aren't talking about adding a feature because of sonar source, my understanding is that this is adding a cyclomatic complexity checker. If and when the feature evolves over time, it would be misleading for it to be called sonar source.

Please note that I didn't do a full code review here, others should do that.

Thank you to you and the board for looking into this and helping us reach the right decision!

please remove the LICENSE.TXT file -- it isn't necessary and it is confusing for people. I would also recommend replacing all the references to sonar source with a functional description of what this patch is doing - we aren't talking about adding a feature because of sonar source,

Done, as much as possible. I'd still prefer to maintain a reference to the spec on which it is based.
Note that it was done because that is what was requested previously by reviewers.

I agree that we should still document that this is based off the Sonar Source paper because it's not a generic implementation but a specific one. I think you've struck a good balance with the current patch.

lebedev.ri updated this revision to Diff 295902.Oct 2 2020, 1:28 PM
lebedev.ri marked 10 inline comments as done.

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 stuff etc. I really appreciate the dilligence here to capture the ideas that went into this code.

My understanding is that the code and documentation are not derived from anyone else's code or documentation -- they are fresh implementations based on a paper written in english prose. If that is the case, please remove the LICENSE.TXT file -- it isn't necessary and it is confusing for people. I would also recommend replacing all the references to sonar source with a functional description of what this patch is doing - we aren't talking about adding a feature because of sonar source, my understanding is that this is adding a cyclomatic complexity checker. If and when the feature evolves over time, it would be misleading for it to be called sonar source.

Please note that I didn't do a full code review here, others should do that.

Thank you to you and the board for looking into this and helping us reach the right decision!

+1.

please remove the LICENSE.TXT file -- it isn't necessary and it is confusing for people. I would also recommend replacing all the references to sonar source with a functional description of what this patch is doing - we aren't talking about adding a feature because of sonar source,

Done, as much as possible. I'd still prefer to maintain a reference to the spec on which it is based.
Note that it was done because that is what was requested previously by reviewers.

I agree that we should still document that this is based off the Sonar Source paper because it's not a generic implementation but a specific one. I think you've struck a good balance with the current patch.

Good!

I've addressed most of the review comments, excluding the one about Microsoft SEH support - i'd prefer to leave that for a follow-up :)
There may be other straddlers anyway.

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
367–368 ↗(On Diff #295879)

This is consistent across this whole FunctionASTVisitor, and is consistent with other RecursiveASTVisitor<>s,
so i'll leave this as-is. If this is wrong, other places should be changed too.

381 ↗(On Diff #295879)

No, BinaryConditionalOperator is explicitly exempt.

388 ↗(On Diff #295879)

Right, it should be.

410 ↗(On Diff #295879)

BlockExprClass as well?

Thank you for the example, that seems obvious & straight-forward to support.
As discussed, we should handle decls thought, not exprs.

Should GNU statement expressions also be treated like a lambda or block?

Hm, i guess it should be.

428 ↗(On Diff #295879)

I'm very much unfamiliar with that extension.
I would prefer to leave it as-is for now.

473 ↗(On Diff #295879)

What about other special member functions like overloaded operators and whatnot?

Those are CXXMethodDecl, but i'll add a test.

How about block declarations?

Right, we should handle them, not blockstatements.

Namespaces is another one I was curious about but suspect may not want to be handled here.

To the best of my knowledge, those can only appear at the global scope.

lebedev.ri updated this revision to Diff 295904.Oct 2 2020, 1:35 PM

Maybe fine tune comments - we count block decls, not block statements/expressions.

aaron.ballman added inline comments.Oct 2 2020, 1:38 PM
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
367–368 ↗(On Diff #295879)

Fine by me, thanks.

381 ↗(On Diff #295879)

I didn't see anything in the paper that talked about this (it's a GNU extension). I feel like anywhere we handle a ternary conditional we should also handle a binary conditional, e.g.,

foo ? foo : bar; // ternary form
foo ? : bar; // binary form of the same thing

but maybe I'm misunderstanding something.

428 ↗(On Diff #295879)

I'm fine handling it in a follow-up. I suspect there may be other esoteric AST nodes we want to add, I just tried to hit the ones that were most obvious to me.

473 ↗(On Diff #295879)

Namespaces is another one I was curious about but suspect may not want to be handled here.

To the best of my knowledge, those can only appear at the global scope.

They can, but I wasn't certain if they'd add to complexity or not given that you can nest them deeply. But now I see that the paper doesn't mention them in terms of C++ but does talk about something similar with JavaScript that suggests they should not contribute to complexity.

lebedev.ri marked 4 inline comments as done.Oct 2 2020, 1:54 PM
lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
381 ↗(On Diff #295879)

As discussed, it does seem like they should at least cause increase in nesting level.
I suspect, they should also cause increase in cognitive complexity IFF they're nested somehow,
because foo ?: bar?: baz doesn't really seem straight-forward.
But likewise, they're an extension, so i think we could fine-tune that later.

428 ↗(On Diff #295879)

Okay, thanks.

473 ↗(On Diff #295879)

What i mean is, can you define a new namespace within the function's body?
I don't believe that to be possible, therefore we shouldn't/couldn't care about that,
because we start at function scope.

aaron.ballman accepted this revision.Oct 2 2020, 2:11 PM

LGTM, thank you for your patience with the process.

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
381 ↗(On Diff #295879)

Agreed, this can be done in a follow-up.

473 ↗(On Diff #295879)

What i mean is, can you define a new namespace within the function's body?

You cannot.

I don't believe that to be possible, therefore we shouldn't/couldn't care about that, because we start at function scope.

Agreed.

lebedev.ri marked 3 inline comments as done.Oct 2 2020, 2:14 PM

@aaron.ballman @JonasToth thank you for the review!
@tonic @lattner thank you for helping resolve the misunderstading :)

! In D36836#2309432, @aaron.ballman wrote:
LGTM, thank you for your patience with the process.

HURRAY!

lattner removed a subscriber: lattner.Oct 2 2020, 2:28 PM
This revision was not accepted when it landed; it landed in state Needs Review.Oct 2 2020, 2:31 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Hm, let me take a look..

Will push likely fix once tests pass locally.

Hm, let me take a look..

Will push likely fix once tests pass locally.

rG1596cc83509342eb37dbfe6b95e906759afc6741, let's see if that does it.