Re: [PATCH 6/9] tty: hvc_console: Fix coding style issues of block comments

[Xiaofei Tan]

Hi Johan，

On 2021/5/20 16:21, Johan Hovold wrote:
> On Tue, May 18, 2021 at 12:01:22PM +0800, Xiaofei Tan wrote:
> > Hi Johan,
> >
> > Thanks for reviewing this patch.
> >
> > On 2021/5/17 22:15, Johan Hovold wrote:
> > > On Mon, May 17, 2021 at 02:37:10PM +0800, Xiaofei Tan wrote:
> > > > Fix coding style issues of block comments, reported by checkpatch.pl.
> > > > Besides, add a period at the end of the sentenses.
> > > >
> > > > Signed-off-by: Xiaofei Tan <tanxiaofei@xxxxxxxxxx>
> > > > ---
> > > >  drivers/tty/hvc/hvc_console.c | 15 ++++++++++-----
> > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> > > > index 39018e5..a61cdf0 100644
> > > > --- a/drivers/tty/hvc/hvc_console.c
> > > > +++ b/drivers/tty/hvc/hvc_console.c
> > > > @@ -177,7 +177,8 @@ static void hvc_console_print(struct console *co, const char *b,
> > > >  			r = cons_ops[index]->put_chars(vtermnos[index], c, i);
> > > >  			if (r <= 0) {
> > > >  				/* throw away characters on error
> > > > -				 * but spin in case of -EAGAIN */
> > > > +				 * but spin in case of -EAGAIN.
> > > > +				 */
> > >
> > > How is this an improvement? First, the multi-line comment style is
> > >
> > > 	/*
> > > 	 * ...
> > > 	 */
> >
> > Yes, mostly we use this style. I can follow it if new version is needed.
>
> This is the preferred style outside of networking.
>
> > BTW, How about add the '/*' check into checkpatch.pl?
>
> Checkpatch already has too many checks IMO and I'm a bit surprised that
> it doesn't check this already. Perhaps it's because you used the -f to
> run checkpatch on in-kernel code, which you should not.
>
> > > Second, that sentence is not capitalised so why do add a period?
> >
> > How about capitalize the sentence, or just remove the period ?
>
> How about just leaving this unchanged?

OK
And I will keep the patch 8/9, and combine space issues into
one new patch, and remove the others.

> > > Third, why are you sending checkpatch.pl cleanups for files outside of
> > > staging?
> >
> > I'm sorry, Is this a rule, or kind of tradition? I've never heard of
> > this before.
>
> Many subsystems reject pure style changes unless you're also doing some
> real changes to the code in question. This is a good default rule, even
> if some maintainers may occasionally accept churn like this.
>
> You appear to be paid to do kernel work. Why don't you start fixing bugs
> or help out reviewing new code instead of sending trivial patches like
> this? We can always need another hand.
>
> But if all you you want is to increase your company patch count then
> please go work in drivers/staging where most trivial style changes are
> currently accepted.
>
> > > Unless doing some real changes to the files in question as well this is
> > > mostly just churn and noise that makes it harder to backport fixes and
> > > do code forensics for no real gain.
> >
> > I'm not sure. But if cleanup patches have made it hard to backport fixes
> > and do code forensics, then the code quality may not be
> > good enough.
>
> No, that has nothing to do with code quality, it's just that you
> introduce noise in the logs and do pointless changes of context which
> makes it harder to use tools like git blame and makes backporting harder
> for no good reason.
>
> Johan
>
> .