Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix option for NETWORKING_BLOCK_COMMENT_STYLE

[Aditya <yashsri421@gmail.com>]

On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
> On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>>
>> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421@gmail.com> wrote:
>>>
>>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
>>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421@gmail.com> wrote:
>>>>>
>>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Currently, checkpatch warns us for files in 'net/' and 'drivers/net',
>>>>>>> if we use an empty '/*' line for comment and contents of comments are
>>>>>>> in next line
>>>>>>>
>>>>>>> E.g., running checkpatch on commit 0d52497ac8ee ("iwlwifi: pcie: remove
>>>>>>> the refs / unrefs from the transport") reports this warning:
>>>>>>>
>>>>>>> WARNING: networking block comments don't use an empty /* line, use /*
>>>>>>> Comment...
>>>>>>> +               /*
>>>>>>> +                * If the TXQ is active, then set the timer, if not,
>>>>>>>
>>>>>>> Provide a fix by appending the current line contents to previous line
>>>>>>> and removing the current line
>>>>>>>
>>>>>>
>>>>>> Patch generally looks good.
>>>>>>
>>>>>> Can you check how many comments in net actually follow that style and how
>>>>>> many follow another style?
>>>>>>
>>>>>>
>>>>>
>>>>> In drivers/net:
>>>>> Wrong style: 14695 lines
>>>>> Correct style: 147961 lines (ie around 10 times)
>>>>>
>>>>> In net/:
>>>>> Wrong style: 5090 lines
>>>>> Correct style: 30485 lines
>>>>>
>>>>
>>>> Can you find out where the wrong style is used?
>>>>
>>>> Maybe the documentation is a bit outdated.
>>>>
>>>> For example, some drivers and some subdirectories might have settled
>>>> for another commenting style.
>>>>
>>>> I guess you can submit the fix option, but it could be that the whole
>>>> rule/feature is broken anyway... so I do not know if that fix is of
>>>> any good...
>>>>
>>>> I think it is better to actually understand, document and encode the
>>>> current rule that applies.
>>>>
>>>> Can you provide an evaluation where the different styles for
>>>> commenting are aggregated in a good way?
>>>> E.g., consistently within a file with style XYZ; mixed style but
>>>> 80-90% are of style ABC; consistent within a directory.
>>>>
>>>> If you think some cases are in the wrong style for some specific
>>>> files, simply send a patch correcting the commenting style and see.
>>>>
>>> Hi Lukas
>>> I have generated the reports regarding the comments used in various
>>> files at net/ and drivers/net.
>>> Directory wise reports:
>>>
>>> net/:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative.txt
>>>
>>> drivers/net:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative.txt
>>>
>>
>> Can you sort that by the overall number of comments per directory?
>>
>>> File wise reports:
>>>
>>> net/:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_file.txt
>>>
>>> drivers/net:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_file.txt
>>>
>>> Some files in 'net/' follow the accepted syntax, such as:
>>> 'net/batman-adv => 985    0'.
>>>
>>> However, some completely not, such as:
>>> 'net/ax25 => 0    103'.
>>>
>>> Some even follow mixed syntax such as:
>>> 'net/netfilter => 87    125'.
>>>
>>> Similarly for drivers/net:
>>> drivers/net/ethernet/mellanox/mlxsw => 1071    0
>>> completely follow accepted syntax.
>>>
>>> drivers/net/wireless/ralink/rt2x00 => 98, 2082
>>> follow unaccepted syntax largely.
>>>
>>> whereas files such as drivers/net/wireless/ath/ath5k => 304, 486
>>> follow mixed syntax.
>>>
>>> It seems to me as the documentation might be outdated, as you had also
>>> suggested earlier, or maybe users are unaware of the documentation and
>>>  just use the syntax for conserving consistency of the code.
>>>
>>> What do you think?
>>>
>>
>> I think it largely depends on the history of the code and developers
>> that take care...
>>
>> We might just start to ask the larger areas to modify the rule for
>> their directory.
>>
> 
> I suggest we start with looking at the (let us say 10) "largest"
> directories with respect to number of comment blocks and then decide
> on a refinement of the rule for those subdirectories:
> 
> A. Directory follows NETWORKING COMMENT STYLE; checkpatch shall warn
> to follow NETWORKING COMMENT STYLE.
> B. Directory follows GENERAL KERNEL COMMENT STYLE; checkpatch shall
> warn to follow GENERAL KERNEL COMMENT STYLE.
> C. Directory is too mixed; checkpatch shall not warn on any comment style.
> 
> We can then start the discussion with the appropriate maintainer (and
> check what else this maintainer might maintain and if that is
> consistent) if they agree or disagree on that.
> 

Hi Lukas
Here is the list I have generated with the directories sorted on the
basis of comment count and labelled the top 10 directories as
following NETWORKING, GENERIC or MIXED comment style(s):

At "drivers/net":
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/drivers_net/cumulative_sorted.txt

At "net":
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/networking_block_comments/net/cumulative_sorted.txt

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees