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

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

[Tom Rini]

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@chromium.org>
Signed-off-by: Tom Rini <trini@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);
 	}
 
 	# prefer strl(cpy|cat) over strn(cpy|cat)
-- 
2.25.1

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

[Simon Glass]

On Tue, 2 Aug 2022 at 06:33, Tom Rini <trini@konsulko.com> 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@chromium.org>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>


> 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);
>         }
>
>         # prefer strl(cpy|cat) over strn(cpy|cat)
> --
> 2.25.1
>

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

[Rasmus Villemoes]

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@chromium.org>
> Signed-off-by: Tom Rini <trini@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.

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.

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.

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

Rasmus

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3065 bytes --]

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@chromium.org>
> > Signed-off-by: Tom Rini <trini@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Simon Glass]

Hi,

On Fri, 5 Aug 2022 at 09:13, Tom Rini <trini@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@chromium.org>
> > > Signed-off-by: Tom Rini <trini@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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4757 bytes --]

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@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@chromium.org>
> > > > Signed-off-by: Tom Rini <trini@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]