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

[Tom Rini]

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.

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.

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

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.

I'll keep this all in mind and come up with better wording for a v2,
thanks.

-- 
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/fe893559/attachment.sig>