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

[Lukas Bulwahn]

On Mon, Nov 30, 2020 at 5:47 PM Aditya <yashsri421 at gmail.com> wrote:
>
> 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.
>

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?

Lukas