Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Thu, 21 Nov 2019 at 16:15, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 21, 2019 at 14:09:41 +0100, Ard Biesheuvel wrote:
> > > > I don't like this at all.
> > > >
> > > > If two expressions happen to evaluate to the same constant, we should
> > > > be able to compare them using C code.
> > > >
> > > > I think we should disable the warning instead.
> > >
> > > I agree with that as a general rule, and agree that disabling the
> > > warning may be preferable in other contexts.
> > >
> > > But I somewhat disagree with referring to *this instance* as two
> > > expressions evaluating to the same constant. This is entirely a
> > > preprocessor event, somewhat obscured by the Pcd syntax.
> > > The test is "has the default baudrate been requested or has a specific
> > > one been requested?".
> >
> > That may be true, but moving from C conditionals to CPP conditionals
> > obscures the code, and reduces test coverage, so I think it should be
> > avoided when possible.
>
> I don't see what test coverage this would change. Even in the NOOPT
> case, only the codegen would be affected - only one of the two cases
> would be reachable. In all other cases, the compiler would go
>   if (115200 != 0) {
>   /* Oh, I'll just ignore any else clauses then ... */
>
> I think the pre-existing code obscures what is actually going on and
> that the change clarifies it (but could be prettified).
>
> Now, of course, why we have a platform-specific (and platform-global)
> override variable to make a library ignore its inputs is another
> interesting question.
>
> So. Rock-paper-scissors or coin-toss?
>

Yes!