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.aka...@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 <s...@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
signature.asc
Description: PGP signature
