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.
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-s...
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()).
Thoughts?
Regards, Simon