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

[Aditya <yashsri421@gmail.com>]

On 30/11/20 10:27 pm, Lukas Bulwahn wrote:
> On Mon, Nov 30, 2020 at 5:47 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 30/11/20 3:40 pm, Lukas Bulwahn wrote:
>>> On Sun, Nov 29, 2020 at 3:58 PM Aditya <yashsri421@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@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?
>>>>
>>>
>>> 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.
>>
> 
> It makes sense to identify the names of those people to start with; I
> would suggest NOT to contact them until we have a first good patch set
> to provide some technical solution.
> 
> I was asking more about how a maintainer can state their preference in
> the repository and how will checkpatch then use this information?
> 
> Did you think about a technical solution for that?
> 

Yes, you are correct. We can probably create a file where we can store
the directories(names) which are inside net or drivers/net, but do not
follow NETWORKING_BLOCK_COMMENT style (something like
networking_block_comment_style.ignore file). Maintainers can add their
directories inside this file.
We can then ignore these files from this class of warning.

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