From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68])
 by mx.groups.io with SMTP id smtpd.web09.25185.1574349386222057369
 for <devel@edk2.groups.io>;
 Thu, 21 Nov 2019 07:16:26 -0800
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@linaro.org header.s=google header.b=TjOoleoJ;
 spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org)
Received: by mail-wr1-f68.google.com with SMTP id 4so1549231wro.7
        for <devel@edk2.groups.io>; Thu, 21 Nov 2019 07:16:26 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=linaro.org; s=google;
        h=mime-version:references:in-reply-to:from:date:message-id:subject:to
         :cc;
        bh=4+TO8I4e92Gyz0VCCaGA4r8Z/0AucZ8LbhBV32Kk8zA=;
        b=TjOoleoJEtLKEru6Z8LmiUycQXMq6y1FCZ6uzPBsVtqIgrmBPGtoLfazlDxL+bejiy
         h5PeSwXAigchaMvwGaZpeszmM1K8znOTLpyZlRcI1KAqc9W2Jj8pZgWWAbYgZ/L2GVaD
         i93XJJzV6Zu0+XGAc5TthWD5ZbKOYMgDfWW3idTmB0QkDF06H/ggN4Fk2m6OuLuy2hTl
         pyZzPYcLWoEuvRiOx3r3i0/DNqVB3QVrFI++vIn7k9oQwD7H4E4IkGx9zYhBzmkjlSHa
         jjFG3Q1NNQKqfSatNtp7La00RUbdbp3eEpwdy6vS9YRiuZrcmDy9BEJgrhzwbR3oPwhW
         1NZw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20161025;
        h=x-gm-message-state:mime-version:references:in-reply-to:from:date
         :message-id:subject:to:cc;
        bh=4+TO8I4e92Gyz0VCCaGA4r8Z/0AucZ8LbhBV32Kk8zA=;
        b=LqnbUWyHdXyg/dNwFPLL6Qym/DbQmGSQiXVPgrPypUjkaz4VuZpqnUf7HphaJp5TOI
         +J0XhkM8XBBBW4AVj3gxBt/PXHzdLwUfrvBm2U4eWio9iWRg2NPDoB0h7Ot/y4hQuHGg
         b3hGNIq8/IeGDzB14InT59tkkSZ/UQq3NWVemt5Punl33b2hBIP3AI7jh88OpxOCnLzd
         5ROVv+iPlCU/ksShZKFdIbur3Ims5F9pI4tytRSRXaZzggNch50yxUNCcHkp1qYDcrdI
         JamWMIavqotOOzL78eFpYPMDbrdUzVExoKlZ0BsXy7WhNsLaYzhxx17n22AWjLkhAuO+
         HF+g==
X-Gm-Message-State: APjAAAVMHZNtkn9XZKJaOJC4lI3KcgcdF4bU/kr+ogy3KmAfmXf/pc5q
	VpRBvryhquqHy+9WMVITQdaHjo8Ei4VwsyZ3ST3ldA==
X-Google-Smtp-Source: APXvYqwGO+/mQ0+v+g4VCPp+KKFrNszhOucQMlYF6P8wFrQohRX5z4rh354dv7hK9byie9UPTvrYU1mWhP3kVmSUZ+o=
X-Received: by 2002:adf:ef45:: with SMTP id c5mr3412662wrp.200.1574349384691;
 Thu, 21 Nov 2019 07:16:24 -0800 (PST)
MIME-Version: 1.0
References: <20190823105539.13260-1-sami.mujawar@arm.com> <20190823105539.13260-19-sami.mujawar@arm.com>
 <20191121123542.GD7359@bivouac.eciton.net> <CAKv+Gu9L_4691P4vVB9RnDnN7DnSGwwgzO-+3E968n1RnZ8P0Q@mail.gmail.com>
 <20191121125315.GF7359@bivouac.eciton.net> <CAKv+Gu8TaUMm2jfpvObH9X9uh1+hawuA7Ft7sW415cVXph5cUQ@mail.gmail.com>
 <20191121151519.GG7359@bivouac.eciton.net>
In-Reply-To: <20191121151519.GG7359@bivouac.eciton.net>
From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
Date: Thu, 21 Nov 2019 16:16:13 +0100
Message-ID: <CAKv+Gu8Bgg-0NPaegJNLOQzTkv_XUo8Ks10+Wzm-7An2KbVvXQ@mail.gmail.com>
Subject: Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>, edk2-devel-groups-io <devel@edk2.groups.io>, 
	Alexei Fedorov <Alexei.Fedorov@arm.com>, Matteo Carlini <Matteo.Carlini@arm.com>, nd <nd@arm.com>
Content-Type: text/plain; charset="UTF-8"

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!