Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
Hi Akashi,
On Mon, 15 Jun 2020 at 18:34, 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
> > > 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
> > > > >
> > > > > 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 have to give up something here. The link error normally reports
where the function was called from. By moving to relying on the
toolchain to sort out what is in the image and what is not, we should
try to do it fully, in my view. A halfway house where we still use
lots of #ifdefs seems seal-defeating.
>
> > ...
> > #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
Yes indeed. Tricky.
>
> > > > 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?
The compiler puts each function into a separate section and the linker
throws away what is not used. So I don't think adding an #ifdef around
an exported function server a purpose except in a small proportion of
cases.
>
> > > > 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?
We don't require people to fix up old code style before submitting a
patch to a file, although checkpatch makes you do it for nearby code.
But I think we should ask contributors to help.
Regards,
Simon
Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
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
> > > 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
> > > > >
> > > > > 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
Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
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
> > 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
> > > >
> > > > 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
Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
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
> 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
> > >
> > > 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
signature.asc
Description: PGP signature
Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
Hi Akashi,
On Sun, 14 Jun 2020 at 20:59, AKASHI Takahiro
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
> >
> > 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.
>
> 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?
> Or,
>
> struct baa baa_list[] = {
> #ifdef CONFIG_xxx
> data_xxx,
> #endif
I'm not sure how to handle this one.
> ...
>
> 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?
>
> Thanks,
> -Takahiro Akashi
Regards,
Simon
Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
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
>
> 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
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
Or,
struct baa baa_list[] = {
#ifdef CONFIG_xxx
data_xxx,
#endif
...
They are harmless and can be ignored, but also annoying.
Can you sophisticate this check?
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?
Thanks,
-Takahiro Akashi
> --
> Tom
Re: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef
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 Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
