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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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.

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

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

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

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

-Takahiro Akashi


> -- 
> Tom