[PATCH] checkpatch.pl: Reword IS_ENABLED() warning
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)
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
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
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.
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
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.
participants (3)
-
Rasmus Villemoes -
Simon Glass -
Tom Rini