linux-kernel - Re: [PATCH v3] checkpatch: add a rule to check general block comment style

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Jul 2021 10:39:29 +0200
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Ani Sinha <ani@...sinha.ca>
Cc:     Joe Perches <joe@...ches.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        anirban.sinha@...ia.com, mikelley@...rosoft.com,
        Andy Whitcroft <apw@...onical.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>
Subject: Re: [PATCH v3] checkpatch: add a rule to check general block comment style

On Mon, Jul 19, 2021 at 10:08 AM Ani Sinha <ani@...sinha.ca> wrote:
>
>
>
> On Mon, 19 Jul 2021, Joe Perches wrote:
>
> > On Mon, 2021-07-19 at 12:25 +0530, Ani Sinha wrote:
> >
> > > I do not see why we cannot add this rule to checkpatch. If the
> > > reviewer likes the other style of commenting they can always ask for a
> > > correction. Having checkpatch agree with Linus' preferred style of
> > > commenting and the preferred documeted style of commenting (which seems to
> > > be the same) does make everything uniform and agreeable.
> >
> > Too many novice developers take checkpatch output as dicta.
> >
> > It's not.
> >
>
> Well those "novice" developers have perhaps worked in companies where
> tooling like pre-commit sanity hooks have provided immense value in
> ensuring certain basic rules and code quality is maintained across the
> board, particulay when the company scales. Existing violations did not
> deter them from adding stricter rules to make sure all future commits
> follow certain patterns. Ofcourse at the end of the day, common sense
> trumps any tooling, goes without saying.
>
> > It's just produces suggestions that should _always_ be taken
> > not very seriously.  Those suggestions should perhaps be
> > considered, but good taste should always override a brainless
> > script.
>
> At the very very least, checkpatch should lay this out in clear terms
> every time this is run. Different communities have different rules and for
> me, I always run all my patches through checkpatch to make sure that the
> patch I sent out of review at least is checkpatch clean. This makes sure
> that I am not violating any obvious code submission rules laid out by that
> community. This is particularly true for kernel community where flaming
> people for even small issues seems to be the culture!
>

You are assuming that there is THE one consistent style in the kernel
repository and within the whole kernel community. But, IMHO, there is
not; the repository is too large and the community is too diverse in
its preferences. Do an evaluation of coding style in the whole
repository and share which one rule is consistently followed in all
directories (and how many are not).

Further, checking as pre-commit hooks requires that a repository is
rather clean wrt. to rule violations, but often in the kernel, it is
not. So, you then need to clean up the existing code, which causes
dangerous large syntactic refactorings and drowns maintainers. That
has been tried, but has been not successful in the past.
Hence, with the current state, the checkpatch tool needs to be handled
much more with care and critical consideration of the state of the
actual code base and the rationales of the rules that checkpatch
points out.

Surely, we need better documentation to point out how to use
checkpatch, what the reasons for certain rules are, and why some
simply need to be ignored on certain files. Evaluations and
contributions are welcome.

> >
> > _Very_ few senior developers really care that much about any
> > particular comment style.
>
> I disagree on this.
>
> >
> > These are the same senior developers that would be burdened
> > with unnecessary patches to review from those novice developers
> > that believe checkpatch should always be followed.
> >
>
> Well for those "novice" developers, the doc tells us to run checkpatch
> and address the complaints :
>
>  Are you sure your patch is free of silly mistakes?  You should always
>    run patches through scripts/checkpatch.pl and address the complaints it
>    comes up with.
>
>
> Anyways it seems this conversation is self serving for the kernel's sr
> developers so that they can take any stance convenient to them.
> There is no value.