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

Aditya yashsri421 at gmail.com
Mon Nov 30 16:47:04 UTC 2020


On 30/11/20 3:40 pm, Lukas Bulwahn wrote:
> On Sun, Nov 29, 2020 at 3:58 PM Aditya <yashsri421 at gmail.com> wrote:
>>
>> On 25/11/20 6:08 pm, Lukas Bulwahn wrote:
>>> On Wed, Nov 25, 2020 at 8:19 AM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
>>>>
>>>> On Wed, Nov 25, 2020 at 8:02 AM Aditya <yashsri421 at gmail.com> wrote:
>>>>>
>>>>> On 22/11/20 12:22 pm, Lukas Bulwahn wrote:
>>>>>> On Thu, Nov 19, 2020 at 11:44 AM Aditya <yashsri421 at gmail.com> wrote:
>>>>>>>
>>>>>>> On 19/11/20 12:30 am, Lukas Bulwahn wrote:
>>>>>>>> On Mi., 18. Nov. 2020 at 18:40, Aditya Srivastava <yashsri421 at 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?
>>
> 
> Obviously, this existing rule needs to be refined.
> 
> Do you have a proposal? Probably, you would like to have each
> maintainer state which comment style is preferred or so.
> 
> How could this be technically done?
> 

We can probably Cc all the corresponding maintainers checking from the
MAINTAINERS file.
Since we are considering only the top 10 directories, this probably
shouldn't be much time consuming.

Or we can probably mail any central maintainer(s) for all net and
drivers/net directories (for eg, someone who decided the comment rule
in the beginning for both the directories) and discuss.

What do you think?

Thanks
Aditya


More information about the Linux-kernel-mentees mailing list