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 <tr...@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.apalodi...@linaro.org> wrote:
> > > >
> > > > On Mon, 1 Nov 2021 at 03:19, Simon Glass <s...@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-...@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
signature.asc
Description: PGP signature
