[Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Dwaipayan Ray]

Hi Lukas and all,
Recently while going through the warnings emitted by checkpatch,
the necessity of a verbose mode came up once again. Joe had
already suggested that a verbose mode would probably be worked
on.

As for how that could be done, that leaves us at a couple of options.
Since writing verbose messages for all warnings aren't possible at once,
there can be an optional extension when emitting messages:

Currently, a warning can be emitted by
WARN("TYPE", "Message")
which could be converted to say:
WARN('TYPE", "Message", "Verbose")

Another way is to leave the original warning emitting syntax intact
and instead go for a dictionary for verbose messages:
our %dict = (
"TYPE1" => "Verbose",
"TYPE2" => "Verbose"
...);

Although this leaves us the ability to customize the verbose output
for each warning of a particular type.

Which do you think would be best? Certainly more options might be
possible, so any new inputs will be nice as well!

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Lukas Bulwahn]

On Fri, Jan 22, 2021 at 10:27 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> Hi Lukas and all,
> Recently while going through the warnings emitted by checkpatch,
> the necessity of a verbose mode came up once again. Joe had
> already suggested that a verbose mode would probably be worked
> on.
>
> As for how that could be done, that leaves us at a couple of options.
> Since writing verbose messages for all warnings aren't possible at once,
> there can be an optional extension when emitting messages:
>
> Currently, a warning can be emitted by
> WARN("TYPE", "Message")
> which could be converted to say:
> WARN('TYPE", "Message", "Verbose")
>
> Another way is to leave the original warning emitting syntax intact
> and instead go for a dictionary for verbose messages:
> our %dict = (
> "TYPE1" => "Verbose",
> "TYPE2" => "Verbose"
> ...);
>
> Although this leaves us the ability to customize the verbose output
> for each warning of a particular type.
>
> Which do you think would be best? Certainly more options might be
> possible, so any new inputs will be nice as well!
>

I think any of the two approaches would work.

But how about a completely different approach?

The verbose message is really an explanation of why kernel developers
think this rule should be followed (what is the history, what does it
try to warn about, what are possible fixes, what are accepted
deviations, ...).

So, this should not be placed into the checkpatch.pl script itself,
but into Documentation.

checkpatch.pl would simply quickly parse the Documentation and present
the content for the specific rules that were violated.

Let's say this Documentation is in
./Documentation/tools/checkpatch.rst, and they contain subsections
with labels/name with the TYPE identifier. Then when a warning of TYPE
is relevant for a specific patch, you print the description from that
one subsection.

In checkpatch itself, you would only need a very generic mechanism then.

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

Re: [Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Dwaipayan Ray]

On Sat, Jan 23, 2021 at 8:44 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 10:27 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> >
> > Hi Lukas and all,
> > Recently while going through the warnings emitted by checkpatch,
> > the necessity of a verbose mode came up once again. Joe had
> > already suggested that a verbose mode would probably be worked
> > on.
> >
> > As for how that could be done, that leaves us at a couple of options.
> > Since writing verbose messages for all warnings aren't possible at once,
> > there can be an optional extension when emitting messages:
> >
> > Currently, a warning can be emitted by
> > WARN("TYPE", "Message")
> > which could be converted to say:
> > WARN('TYPE", "Message", "Verbose")
> >
> > Another way is to leave the original warning emitting syntax intact
> > and instead go for a dictionary for verbose messages:
> > our %dict = (
> > "TYPE1" => "Verbose",
> > "TYPE2" => "Verbose"
> > ...);
> >
> > Although this leaves us the ability to customize the verbose output
> > for each warning of a particular type.
> >
> > Which do you think would be best? Certainly more options might be
> > possible, so any new inputs will be nice as well!
> >
>
> I think any of the two approaches would work.
>
> But how about a completely different approach?
>
> The verbose message is really an explanation of why kernel developers
> think this rule should be followed (what is the history, what does it
> try to warn about, what are possible fixes, what are accepted
> deviations, ...).
>
> So, this should not be placed into the checkpatch.pl script itself,
> but into Documentation.
>
> checkpatch.pl would simply quickly parse the Documentation and present
> the content for the specific rules that were violated.
>
> Let's say this Documentation is in
> ./Documentation/tools/checkpatch.rst, and they contain subsections
> with labels/name with the TYPE identifier. Then when a warning of TYPE
> is relevant for a specific patch, you print the description from that
> one subsection.
>
> In checkpatch itself, you would only need a very generic mechanism then.
>

That sounds great to me!
Even I had it in mind but thought maybe checkpatch could be less
reliant on other
files. But this also solves our purpose for an external documentation
for checkpatch
rules, so its a kinda win win situation.

I think maybe we will need a special structure for the documentation file itself
but its certainly doable.

Do you think I should go on to propose this idea to Joe now?

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

Re: [Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Lukas Bulwahn]

[-- Attachment #1.1: Type: text/plain, Size: 2812 bytes --]

Sounds good to me.

Dwaipayan Ray <dwaipayanray1@gmail.com> schrieb am Sa., 23. Jan. 2021,
11:12:

> On Sat, Jan 23, 2021 at 8:44 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
> wrote:
> >
> > On Fri, Jan 22, 2021 at 10:27 PM Dwaipayan Ray <dwaipayanray1@gmail.com>
> wrote:
> > >
> > > Hi Lukas and all,
> > > Recently while going through the warnings emitted by checkpatch,
> > > the necessity of a verbose mode came up once again. Joe had
> > > already suggested that a verbose mode would probably be worked
> > > on.
> > >
> > > As for how that could be done, that leaves us at a couple of options.
> > > Since writing verbose messages for all warnings aren't possible at
> once,
> > > there can be an optional extension when emitting messages:
> > >
> > > Currently, a warning can be emitted by
> > > WARN("TYPE", "Message")
> > > which could be converted to say:
> > > WARN('TYPE", "Message", "Verbose")
> > >
> > > Another way is to leave the original warning emitting syntax intact
> > > and instead go for a dictionary for verbose messages:
> > > our %dict = (
> > > "TYPE1" => "Verbose",
> > > "TYPE2" => "Verbose"
> > > ...);
> > >
> > > Although this leaves us the ability to customize the verbose output
> > > for each warning of a particular type.
> > >
> > > Which do you think would be best? Certainly more options might be
> > > possible, so any new inputs will be nice as well!
> > >
> >
> > I think any of the two approaches would work.
> >
> > But how about a completely different approach?
> >
> > The verbose message is really an explanation of why kernel developers
> > think this rule should be followed (what is the history, what does it
> > try to warn about, what are possible fixes, what are accepted
> > deviations, ...).
> >
> > So, this should not be placed into the checkpatch.pl script itself,
> > but into Documentation.
> >
> > checkpatch.pl would simply quickly parse the Documentation and present
> > the content for the specific rules that were violated.
> >
> > Let's say this Documentation is in
> > ./Documentation/tools/checkpatch.rst, and they contain subsections
> > with labels/name with the TYPE identifier. Then when a warning of TYPE
> > is relevant for a specific patch, you print the description from that
> > one subsection.
> >
> > In checkpatch itself, you would only need a very generic mechanism then.
> >
>
> That sounds great to me!
> Even I had it in mind but thought maybe checkpatch could be less
> reliant on other
> files. But this also solves our purpose for an external documentation
> for checkpatch
> rules, so its a kinda win win situation.
>
> I think maybe we will need a special structure for the documentation file
> itself
> but its certainly doable.
>
> Do you think I should go on to propose this idea to Joe now?
>
> Thanks,
> Dwaipayan.
>

[-- Attachment #1.2: Type: text/html, Size: 3967 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

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

Re: [Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Dwaipayan Ray]

On Sat, Jan 23, 2021 at 6:16 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> Sounds good to me.
>
> Dwaipayan Ray <dwaipayanray1@gmail.com> schrieb am Sa., 23. Jan. 2021, 11:12:
>>
>> On Sat, Jan 23, 2021 at 8:44 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>> >
>> > On Fri, Jan 22, 2021 at 10:27 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>> > >
>> > > Hi Lukas and all,
>> > > Recently while going through the warnings emitted by checkpatch,
>> > > the necessity of a verbose mode came up once again. Joe had
>> > > already suggested that a verbose mode would probably be worked
>> > > on.
>> > >
>> > > As for how that could be done, that leaves us at a couple of options.
>> > > Since writing verbose messages for all warnings aren't possible at once,
>> > > there can be an optional extension when emitting messages:
>> > >
>> > > Currently, a warning can be emitted by
>> > > WARN("TYPE", "Message")
>> > > which could be converted to say:
>> > > WARN('TYPE", "Message", "Verbose")
>> > >
>> > > Another way is to leave the original warning emitting syntax intact
>> > > and instead go for a dictionary for verbose messages:
>> > > our %dict = (
>> > > "TYPE1" => "Verbose",
>> > > "TYPE2" => "Verbose"
>> > > ...);
>> > >
>> > > Although this leaves us the ability to customize the verbose output
>> > > for each warning of a particular type.
>> > >
>> > > Which do you think would be best? Certainly more options might be
>> > > possible, so any new inputs will be nice as well!
>> > >
>> >
>> > I think any of the two approaches would work.
>> >
>> > But how about a completely different approach?
>> >
>> > The verbose message is really an explanation of why kernel developers
>> > think this rule should be followed (what is the history, what does it
>> > try to warn about, what are possible fixes, what are accepted
>> > deviations, ...).
>> >
>> > So, this should not be placed into the checkpatch.pl script itself,
>> > but into Documentation.
>> >
>> > checkpatch.pl would simply quickly parse the Documentation and present
>> > the content for the specific rules that were violated.
>> >
>> > Let's say this Documentation is in
>> > ./Documentation/tools/checkpatch.rst, and they contain subsections
>> > with labels/name with the TYPE identifier. Then when a warning of TYPE
>> > is relevant for a specific patch, you print the description from that
>> > one subsection.
>> >
>> > In checkpatch itself, you would only need a very generic mechanism then.
>> >
>>
>> That sounds great to me!
>> Even I had it in mind but thought maybe checkpatch could be less
>> reliant on other
>> files. But this also solves our purpose for an external documentation
>> for checkpatch
>> rules, so its a kinda win win situation.
>>
>> I think maybe we will need a special structure for the documentation file itself
>> but its certainly doable.
>>
>> Do you think I should go on to propose this idea to Joe now?
>>

Hey Joe!
We came across some ideas for a verbose mode in checkpatch. Two basic
ideas were to modify the warning emitting syntax to include an optional
verbose text, say WARN("TYPE", "MESSAGE", "VERBOSE_MSG"), and
another was adding a dictionary of some sorts which contained the verbose
description of the types.

But the most interesting idea here is to use an external documentation file,
say Documentation/tools/checkpatch.rst which can be parsed by checkpatch
and consecutively displays the verbose messages while emitting warnings.
This serves two purposes:
o Adding the verbose mode
o Adding an external documentation for checkpatch

Is this idea acceptable to you?

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] Addition of verbose mode to checkpatch

[Joe Perches]

On Sat, 2021-01-23 at 19:41 +0530, Dwaipayan Ray wrote:

> But the most interesting idea here is to use an external documentation file,
> say Documentation/tools/checkpatch.rst which can be parsed by checkpatch
> and consecutively displays the verbose messages while emitting warnings.
> This serves two purposes:
> o Adding the verbose mode
> o Adding an external documentation for checkpatch
> 
> Is this idea acceptable to you?

Very much so, yes.  An external file is a much better choice.

The idea that it could also be .rst documentation is interesting.

I presume the checkpatch documentation would the first section of the
doc and the verbose descriptions would be another section/chapter.

Try it and see what you come up with.

Implementation wise, please have checkpatch read the file only if
--verbose is enabled and store the verbose description contents so
that the file is only read once.


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