Stuck clang-tidy check review, legal issues?

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Stuck clang-tidy check review, legal issues?

Eric Fiselier via cfe-dev
Hi.

I have the following Differential
https://reviews.llvm.org/D36836

> SUMMARY
> ...
> 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 [[ https://www.sonarsource.com/docs/
CognitiveComplexity.pdf | 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.

As you can see, the implementation is based on a specification.

That Differential was created on 17 Aug, so ~50 days ago. In all this time,
there has been basically no feedback. Mid-September, thanks to Aaron Ballman,
the first review was done by Jonas Toth. Thanks to both of them!

After that, the code owner, Alexander Kornienko, has left a short message
questioning the legal status of that code
https://reviews.llvm.org/D36836#877636

In my reply https://reviews.llvm.org/D36836#877642 i have shared the email
exchange which i believe supports my view that it is okay to have this in
LLVM. Since then, there has been no feedback.

Questions that i'd love to get addressed by this mail
* Am i doing something wrong?
* Am i, as not a lawyer, wrong in my view about the code status?
* Can somebody please review that code? :)

Roman.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Stuck clang-tidy check review, legal issues?

Eric Fiselier via cfe-dev
On Thu, Oct 5, 2017 at 10:08 AM, Roman Lebedev <[hidden email]> wrote:

> Hi.
>
> I have the following Differential
> https://reviews.llvm.org/D36836
>
>> SUMMARY
>> ...
>> 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 [[ https://www.sonarsource.com/docs/
> CognitiveComplexity.pdf | 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.
>
> As you can see, the implementation is based on a specification.
>
> That Differential was created on 17 Aug, so ~50 days ago. In all this time,
> there has been basically no feedback. Mid-September, thanks to Aaron Ballman,
> the first review was done by Jonas Toth. Thanks to both of them!
>
> After that, the code owner, Alexander Kornienko, has left a short message
> questioning the legal status of that code
> https://reviews.llvm.org/D36836#877636
>
> In my reply https://reviews.llvm.org/D36836#877642 i have shared the email
> exchange which i believe supports my view that it is okay to have this in
> LLVM. Since then, there has been no feedback.

I provided feedback on 9/28 that basically suggests we should add
something to our license files. I think the email is sufficient to
demonstrate we're likely in the clear, but an email as part of a
review chain is not sufficient by itself -- we need to capture that
information elsewhere which people downloading the source will have
access to.

> Questions that i'd love to get addressed by this mail
> * Am i doing something wrong?

No.

> * Am i, as not a lawyer, wrong in my view about the code status?

Not to my knowledge, but I'm also not a lawyer.

> * Can somebody please review that code? :)

When dealing with obscure licensing questions, everything takes longer
than normal because we need to be careful to get it right. When I
worked on the licensing stuff with the CERT checkers I ran into
similar delays. My recommendation is to work on the license file stuff
that I mentioned in the review and then re-ping once that's completed.
It may take a few tries to get it right because we have *some*
precedent, but this is far from a regular occurrence.

~Aaron
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev