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.
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;
Rasmus
