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

[Tom Rini]

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 at 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 at 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:
...
#else
static inline 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.

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

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

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