On Tue, Oct 03, 2023 at 01:37:14AM +0200, Rasmus Villemoes wrote:
On 03/10/2023 00.46, Tom Rini wrote:
On Tue, Oct 03, 2023 at 12:27:25AM +0200, Heinrich Schuchardt wrote:
error = stdio_register(dev); -#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)
- /* check if this is the standard input device */
- if (!error && strcmp(env_get("stdin"), dev->name) == 0) {
/* reassign the console */if (OVERWRITE_CONSOLE ||console_assign(stdin, dev->name))return -1;
- if ((IS_ENABLED(SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)) &&
!error) {const char *cstdin;/* check if this is the standard input device */cstdin = env_get("stdin");if (cstdin && !strcmp(cstdin, dev->name)) {/* reassign the console */if (OVERWRITE_CONSOLE ||console_assign(stdin, dev->name))return -1;}-#else
- error = error;
-#endif
return 0; }
This is an example I think of where #if is more readable.
I kind of agree, but at the same time, this could just be rewritten to avoid that extra indentation. Turn the condition around and make it an early return, then the rest of the function need not be indented.
Yes, both the indentation and having to use "CONFIG_SPL_BUILD" to start with (which is a special case!).
I also think the conversion above is buggy (loses a ! on the SPL part), but also, the condition is needlessly convoluted to begin with. !defined(CONFIG_SPL_BUILD) implies ENV_SUPPORT, so the condition is equivalent to just CONFIG_IS_ENABLED(ENV_SUPPORT). So I'd add a
if (!CONFIG_IS_ENABLED(ENV_SUPPORT) return 0;
Changes like that would make things overall better, agreed.