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. -- Tom
signature.asc
Description: PGP signature
