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

["Leif Lindholm" <leif.lindholm@linaro.org>]

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?

/
    Leif