[PATCH 6/6] checkpatch.pl: Request if() instead #ifdef

[Tom Rini <trini@konsulko.com>]

On Tue, Jun 16, 2020 at 09:34:30AM +0900, AKASHI Takahiro wrote:
> On Mon, Jun 15, 2020 at 10:34:56AM -0400, Tom Rini wrote:
> > On Sun, Jun 14, 2020 at 09:58:48PM -0600, Simon Glass wrote:
> > > Hi Akashi,
> > > 
> > > On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Thu, Jun 04, 2020 at 07:39:35PM -0400, Tom Rini wrote:
> > > > > On Fri, May 22, 2020 at 04:32:40PM -0600, Simon Glass wrote:
> > > > >
> > > > > > There is a lot of use of #ifdefs in U-Boot. In an effort reduce this,
> > > > > > suggest using the compile-time construct.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > Applied to u-boot/master, thanks!
> > > >
> > > > This check is simple, but IMHO, too simple.
> > > > It will generate false-positive, or pointless, warnings
> > > > for some common use cases. Say,
> > > >
> > > > In an include header,
> > > > #ifdef CONFIG_xxx
> > > > extern int foo_data;
> > > > int foo(void);
> > > > #endif
> > > 
> > > We should try to avoid this in header files. But I sent a patch
> > > earlier today to turn off the check for header files and device tree.
> > 
> > Right, in a header that's a bad idea unless it's:
> 
> I'm not sure that it is a so bad idea; I think that it will
> detect some configuration error immediately rather than
> at the link time.

We prefer link time failures as -Werror is not the default in a regular
build.

> > ...
> > #else
> > static inline foo(void) {}
> > #endif
> 
> Well, in this case, a corresponding C file often has a definition like:
> #ifdef CONFIG_xxx
> int foo(void) {
>     ...
> }
> #endif

Right, and that's fine.  But headers do not get guards around functions
unless it's else-inline-to-nop-it-out.

> > > > Or in a C file (foo_common.c),
> > > > #ifdef CONFIG_xxx_a
> > > > int foo_a(void)
> > > > ...
> > > > #endif
> > > > #ifdef CONFIG_xxx_b
> > > > int foo_b(void)
> > > > ...
> > > > #endif
> > > >
> > > 
> > > Perhaps the if() could be inside those functions in some cases?
> > 
> > Yeah, that's less clearly an example of something bad.
> 
> Again, I'm not sure that it is a bad idea. Such a use can be
> seen quite often in library code where there are many configurable
> options.
> The only way to avoid such a style of coding is that we would
> put each function into a separate C file even if it can be
> very small. It also requires more (common/helper) functions, which are
> essentially local to that library, to be declared as global.
> 
> Is this what you want?

It comes down to what the code reads best as, yes.  A checkpatch error
isn't a fatal you must fix it error.  But you must be able to explain
why it's wrong.  And I think we're getting away from the main point
here.  Generally, #ifdef CONFIG_FOO .... #endif, in a function is ugly
and we can do better.  It also means better code analysis as I believe
some tools will still evaluate if (0) { ... } but will not evaluate #if
0 ... #endif.

> > > > Or,
> > > >
> > > > struct baa baa_list[] = {
> > > > #ifdef CONFIG_xxx
> > > >         data_xxx,
> > > > #endif
> > > 
> > > I'm not sure how to handle this one.
> > 
> > Rasmus' series to add CONFIG_IS_ENABLED(SYM, true, false) stuff would be
> > handy here.
> 
> Ah, I didn't notice that. We can now have the code like:
> struct baa baa_list[] = {
>     ...
>     CONFIG_IS_ENABLED(xxx, (data_xxx,))
>     ...
> }
> 
> ## I think the comma after 'data_xxx' is required, isn't it?
> 
> But what is the merit?
> 
> And, data_xxx itself has to be declared anyway like:
> #ifdef CONFIG_xxx
> struct baa data_xxx = {
>     ...
> };
> #endif

We _could_ have that yes, he's posted an RFC I need to reply to
directly.  As you would probably also need a __maybe_unused on the
struct itself.  Is that better?

> > > > ...
> > > >
> > > > They are harmless and can be ignored, but also annoying.
> > > > Can you sophisticate this check?
> > > 
> > > Yes I agree we should avoid false negatives. It is better not to have
> > > a check than have one that is unreliable.
> > > 
> > > >
> > > > In addition, if I want to stick to this rule, there can co-exist
> > > > an "old" style and "new" style of code in a single file.
> > > > (particularly tons of examples in UEFI subsystem)
> > > >
> > > > How should we deal with this?
> > > 
> > > Convert it?
> > 
> > Yes, code should be cleaned up and converted to using if (...) when
> > possible.  That we have new code that doesn't make use of this is
> > because we didn't have tooling warning about when it wasn't used.
> 
> So if we want to add a new commit that complies with this rule while
> the file to which it will be applied has an old style of code,
> do you *require* that this existing file should be converted first
> in any case?

I honestly don't know.  Is it a problem to look over the code and make
use of if (IS_ENABLED(...)) { ... } when it would make the code read
better and get better analysis?

-- 
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/20200615/4232080d/attachment.sig>