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

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Nov 30 10:10:09 UTC 2020


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?

Lukas


More information about the Linux-kernel-mentees mailing list