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

[Simon Glass]

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)
>
> 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.

>
> > (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.

>
> 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.

>
> 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.

Regards,
SImon