Re: [PATCH] Documentation: checkpatch: Tweak BIT() macro include

From: Dwaipayan Ray <dwaipayanray1@gmail.com>
To: Andrew Jeffery <andrew@aj.id.au>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	Joe Perches <joe@perches.com>, Jonathan Corbet <corbet@lwn.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	openbmc@lists.ozlabs.org, Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH] Documentation: checkpatch: Tweak BIT() macro include
Date: Thu, 20 May 2021 14:47:28 +0530	[thread overview]
Message-ID: <CABJPP5C7ZokRycaE0aAvUv3BfOJqOvPyqn-P0bbPdyCfnuuESw@mail.gmail.com> (raw)
In-Reply-To: <a0d1f44a-c8ff-4108-af34-6455b5683262@www.fastmail.com>

On Thu, May 20, 2021 at 12:55 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Thu, 20 May 2021, at 16:28, Lukas Bulwahn wrote:
> > On Thu, May 20, 2021 at 3:57 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > While include/linux/bitops.h brings in the BIT() macro, it was moved to
> > > include/linux/bits.h in [1]. Since [1] BIT() has moved again into
> > > include/vdso/bits.h via [2].
> > >
> > > I think the move to the vDSO header can be considered a implementation
> > > detail, so for now update the checkpatch documentation to recommend use
> > > of include/linux/bits.h.
> > >
> > > [1] commit 8bd9cb51daac ("locking/atomics, asm-generic: Move some macros from <linux/bitops.h> to a new <linux/bits.h> file")
> > > [2] commit 3945ff37d2f4 ("linux/bits.h: Extract common header for vDSO")
> > >
> > > Cc: Jiri Slaby <jirislaby@kernel.org>
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> >
> > Looks sound to me.
> >
> > I would prefer a bit of word-smithing the commit message by just
> > removing the references:
> >
> > So:
> >
> > > While include/linux/bitops.h brings in the BIT() macro, it was moved to
> > > include/linux/bits.h in commit 8bd9cb51daac ("locking/atomics, asm-generic: Move some macros from <linux/bitops.h> to a new <linux/bits.h> file"). Since that commit, BIT() has moved again into
> > > include/vdso/bits.h via commit 3945ff37d2f4 ("linux/bits.h: Extract common header for vDSO").
> > >
> > > I think the move to the vDSO header can be considered a implementation
> > > detail, so for now update the checkpatch documentation to recommend use
> > > of include/linux/bits.h.
> > >
> >
> > And then drop references [1] and [2].
> >
> > Andrew, what do you think?
>
> I mostly did this because initially I wrapped the commit message and
> checkpatch spat out errors when it failed to properly identify the
> commit description for [1]. But, leaving the description unwrapped
> inline in the text feels untidy as it's just a work-around to dodge a
> shortcoming of checkpatch.
>
> With the reference style the long line moves out of the way and
> checkpatch can identify the commit descriptions, at the expense of
> complaints about line length instead. But the line length issue was
> only a warning and so didn't seem quite so critical.
>
> While the referencing style is terse I felt it was a reasonable
> compromise that didn't involve fixing checkpatch to fix the checkpatch
> documentation :/
>

Hey,
Can you share which wrap around caused the checkpatch errors
to be emitted? We can try to fix that.

I was able to wrap it without checkpatch complaining. You might consider
replacing it with this if you wish?

While include/linux/bitops.h brings in the BIT() macro, it was moved to
include/linux/bits.h in commit 8bd9cb51daac ("locking/atomics, asm-generic:
Move some macros from <linux/bitops.h> to a new <linux/bits.h> file").

Since that commit BIT() has moved again into include/vdso/bits.h via
commit 3945ff37d2f4 ("linux/bits.h: Extract common header for vDSO").

I think the move to the vDSO header can be considered an implementation
detail, so for now update the checkpatch documentation to recommend use
of include/linux/bits.h.

Thanks,
Dwaipayan.