[PATCH] checkpatch.pl: Reword IS_ENABLED() warning

[Tom Rini]

On Fri, Aug 05, 2022 at 10:48:17AM -0600, Simon Glass wrote:
> Hi,
> 
> On Fri, 5 Aug 2022 at 09:13, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Wed, Aug 03, 2022 at 09:43:00AM +0200, Rasmus Villemoes wrote:
> > > On 02/08/2022 14.33, Tom Rini wrote:
> > > > While there are good reasons to use 'if (IS_ENABLED(CONFIG_...))' for
> > > > runtime tests, there's equally good reasons to use '#ifdef CONFIG_...'
> > > > for build time tests.  Reword this message to hopefully avoid confusion.
> > > >
> > > > Cc: Simon Glass <sjg at chromium.org>
> > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > ---
> > > >  scripts/checkpatch.pl | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index fe13e265a3fe..2d737bdb991b 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -2621,7 +2621,7 @@ sub u_boot_line {
> > > >     # use if instead of #if
> > > >     if ($realfile =~ /\.c$/ && $line =~ /^\+#if.*CONFIG.*/) {
> > > >             WARN("PREFER_IF",
> > > > -                "Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible\n" . $herecurr);
> > > > +                "Possible runtime #ifdef, see if using 'if (IS_ENABLED(CONFIG...))' instead is possible\n" . $herecurr);
> > > >     }
> > >
> > > Eh, there obviously is no such thing as a "runtime #ifdef", so I don't
> > > think this is an improvement.
> >
> > I'm not entirely happy with it either.
> >
> > > Nor is the form "if (IS_ENABLED(...))" really a run-time thing anyway,
> > > we certainly rely on the compiler to fold away any occurrence of
> > > build-time constants inside if() and while() and similar. So also the
> > > commit message is somewhat misleading.
> >
> > True, but if (IS_ENABLED(...)) is only useful in runtime.
> >
> > > The reason the if() form is preferred is to have the compiler check the
> > > contained code for syntactic correctness, and there are two main ways
> > > (that I can think of off hand) that the #if form can be necessary:
> > >
> > > (1) The code references fields of structs that only exist when that
> > > CONFIG is enabled, probably to save some memory when they are not needed.
> 
> We can do:
> 
> struct fred {
> CONFIG_IS_ENABLED(MARY, (int member;))
> };
> 
> I've been thinking of moving to this, actually, perhaps with a new
> macro called CONFIG_MEMBER(MARY, int member)

I think that's less readable.

> > It's also for when rev A has one set of registers and rev B has another,
> > but we can still use the same overall struct to talk with the device.
> > There's a lot of this.
> 
> That's not very nice though. It should really happen at runtime with a
> compatible string.

We can see what makes sense when they come up, but I'm not sure this
fits.

> > > (2) The code references functions which are only declared when that
> > > CONFIG is enabled.
> > >
> > > Now, there's usually not much to do about (1), except one could ask if
> > > the memory saving is worth it (obviously if its some 4K debug array, not
> > > so obviously if it's just two extra ints that go unused when
> > > !CONFIG_FOO). For (2), I believe all guards of declarations in headers
> > > by #ifdefs are bugs that need to be fixed, so that the if() form can be
> > > used to elide emitting calls to such functions. The same goes for
> > > guarding whole struct definitions or enums and similar.
> 
> Yes this has been my policy, at least, for quite a while. Sometimes we
> need to split up a header to avoid circular dependencies.
> 
> However we often use #ifdef in the header to provide two different
> versions of functions, e.g. one that is just a nop, like clk_request()
> 
> In fact I think #ifdefs should only be in header files, in the ideal case.

Right, these cases are fine, but some of the old cases we still have are
just guards around prototypes, which do need to be removed.

> > We need guards around the functions themselves otherwise we would get
> > defined but unused on static functions. There's other cases where it
> > could be argued that the file itself needs to be split so that there's
> > less code that might be unrelated in it.
> 
> For the first bit, __maybe_unused can help.

When it's better for readability, yes. Sometimes it's not and it's
better to just if out the functions.

> > I'll keep this all in mind and come up with better wording for a v2,
> > thanks.
> 
> Actually as well as this patch it seems we need something that
> explains the #ifdef policy (and tips) in more detail.

Yes, getting something put in to doc/develop/ would be good.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220805/6cc73847/attachment.sig>