From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 15 Jun 2020 21:21:34 -0400 Subject: [PATCH 6/6] checkpatch.pl: Request if() instead #ifdef In-Reply-To: <20200616003430.GA15478@laputa> References: <20200522223240.187032-1-sjg@chromium.org> <20200522163226.6.I1e88f035ad8ba4d943ea914eaa536d4cbdc06495@changeid> <20200604233935.GO32287@bill-the-cat> <20200615025857.GA20498@laputa> <20200615143456.GB24893@bill-the-cat> <20200616003430.GA15478@laputa> Message-ID: <20200616012134.GR24893@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: