Re: [PATCH 02/31] kconfig: Add support for conditional values

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3767 bytes --]

On Wed, Jan 12, 2022 at 03:22:51PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 12 Jan 2022 at 14:56, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jan 12, 2022 at 02:28:21PM -0700, Simon Glass wrote:
> > > Hi Ilias,
> > >
> > > On Mon, 1 Nov 2021 at 01:05, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > On Mon, 1 Nov 2021 at 03:19, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > At present if an optional Kconfig value needs to be used it must be
> > > > > bracketed by #ifdef. For example, with this Kconfig setup:
> > > > >
> > > > > config WIBBLE
> > > > >         bool "Support wibbles, the world needs more wibbles"
> > > > >
> > > > > config WIBBLE_ADDR
> > > > >         hex "Address of the wibble"
> > > > >         depends on WIBBLE
> > > > >
> > > > > then the following code must be used:
> > > > >
> > > > >  #ifdef CONFIG_WIBBLE
> > > > >  static void handle_wibble(void)
> > > > >  {
> > > > >         int val = CONFIG_WIBBLE_ADDR;
> > > > >
> > > > >         ...
> > > > >  }
> > > > >  #endif
> > > > >
> > > >
> > > > The example here might be a bit off and we might need this for int
> > > > related values. Was this function handle_wibble() supposed to return
> > > > an int or not?  We could shield the linker easier here without adding
> > > > macros. Something along the lines of
> > > > static void handle_wibble(void)
> > > > {
> > > > #ifdef CONFIG_WIBBLE
> > > > int val = CONFIG_WIBBLE_ADDR;
> > > > #endif
> > > > }
> > > >
> > > > In that case you don't an extra ifdef to call handle_wibble().
> > > > Personally I find this easier to read.
> > >
> > > But how does that help with the problem here? I am trying to avoid
> > > using preprocessor macros in this case.
> >
> > I'm not sure I see a problem here.  A number of the finish-converting-X
> > that I did recently had a guard symbol first because usage wasn't fully
> > converted but really everyone using that area of code needed to set the
> > value, or use the default.
> >
> > There might be some cases where we do still need a guard symbol because
> > usage is in common code and maybe shouldn't be, but instead moved to
> > other usage-specific files.
> >
> > I also think I've seen cases where doing:
> > if (CONFIG_EVALUATES_TO_ZERO) {
> >   ...
> > }
> >
> > takes more space in the binary than an #ifdef does.
> 
> I'd like to see that.

It's one of those fairly maddening cases when it happens.  I _think_
it's more correctly shown as:
if (CONFIG_FOO && bar->baz)
which yes, was excluded but resulted in different code optimization?  I
went to far as to compare the disassembly and just left scratching my
head why it changed.

> > And finally for the moment, we also have many cases where zero is a
> > valid value.  That's what leads to potentially harder to read code or
> > needing a guard, I think.
> 
> The specific case where this is (to be) used is here:
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20211101011734.1614781-14-sjg@chromium.org/
> 
> addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED, CONFIG_BLOBLIST_ADDR);
> 
> This is because BLOBLIST_ADDR depends on BLOBLIST_FIXED (it is
> meaningless to have an address if the bloblits is allocated).
> 
> One fix is to always have an address, and set it to 0 by default (and
> not use it) when BLOBLIST_FIXED is not enabled.
> 
> But it does lead to a strange Kconfig since options are present which
> are not really used.
> 
> Another option is to add an accessor to the header file, as is down
> with global_data (e.g. as is done with gd_of_root()).

Yeah, lets solve this any other way.  If we can do it local to the file,
that's best.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]