From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web10.23492.1574341784509089613 for ; Thu, 21 Nov 2019 05:09:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=xeepLHfd; spf=pass (domain: linaro.org, ip: 209.85.128.66, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f66.google.com with SMTP id l17so3670719wmh.0 for ; Thu, 21 Nov 2019 05:09:44 -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=4s9A/E9beK5YCRY/MSQ1ObE9H8IQjio7+ibk48mRkHY=; b=xeepLHfdrnT+QJ3xsnLmE2a6Avx9KDxnEtmvyB5L3r1SxY4OwTZ2CJUrZRpEqTK82k nLSbXiMG9lMUPwBBpU1m2HakKA0g1/gy8KMo6jI9vo/AGzffGfPGi8CKvR8cC0VVNcbD 3m4/i566oelzXIUqfU2SWgCLJYlmOaHZCoAcYa97/flFSHn8Rfw1pU0vEz5bF8mQNbjq gKuMeli/K4Zd5LXvF8y4BnRKH1bbFSVDx/y2cZPR6i/jmgQxNfO8XZlUKsEBlyxP7r/3 4uJejrvELW9AcSJX4nwxCagPUTFP/A8J119DwOJR3JyBd7A2mykfKRc8DSQO6NbWNLna V9uQ== 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=4s9A/E9beK5YCRY/MSQ1ObE9H8IQjio7+ibk48mRkHY=; b=S9HvHb6i3hZZMC5/3ERg/96y4PHIiAGV5SQq1P4QFAg/HQNvl3BfaI7VEsD9R+r2NF P6KCNkRjjKevUUgxw8CAgDqi5/Hlv76Cm0GITlIOFca1pbZBzT6/jvqfgM19MZougNqs ZlQnhyEDxy9cde1sM8J7CtJFNNkD9MI2Uo0tNi+zWcBaqWxJ21LeM0FzaGGR/pU77/iY 43uU6PHOW8pef8uC48oPtVfDSq3n7QlOcwIu09P1O8JcP/cVDs5Ann5LOQ73NZicIbhb qTjVCexb75NbNtRnLfDKnlnETDESJ8R2BxcRQu/2jCPFtOj1pwLHYziu8VPC5zIIqEOv 4RwQ== X-Gm-Message-State: APjAAAXYST81KpUz5gJFvRKHeTRJlgOpIVSm/zf1ALKJpC8imLL5C/Co f7dRFsu2BqKJOVkirvJuy9UypzSg4uJxRMzIfo5sEQ== X-Google-Smtp-Source: APXvYqw5io/gM/tAXZOLvSBIuPtRr9a258vr78wKlMxmbaM9sGLqf3/aRQX04NTszc69dsxp0ffzRJO8GQJFFoRVUxU= X-Received: by 2002:a1c:b1c3:: with SMTP id a186mr10014913wmf.10.1574341782966; Thu, 21 Nov 2019 05:09:42 -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> <20191121125315.GF7359@bivouac.eciton.net> In-Reply-To: <20191121125315.GF7359@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Thu, 21 Nov 2019 14:09:41 +0100 Message-ID: Subject: Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning To: Leif Lindholm Cc: Sami Mujawar , edk2-devel-groups-io , Alexei Fedorov , Matteo Carlini , nd Content-Type: text/plain; charset="UTF-8" On Thu, 21 Nov 2019 at 13:53, Leif Lindholm wrote: > > On Thu, Nov 21, 2019 at 13:39:55 +0100, Ard Biesheuvel wrote: > > On Thu, 21 Nov 2019 at 13:35, Leif Lindholm wrote: > > > > > > On Fri, Aug 23, 2019 at 11:55:38 +0100, Sami Mujawar wrote: > > > > The VS2017 compiler reports 'warning C6326: potential > > > > comparison of a constant with another constant' when > > > > a fixed PCD value is compared with a constant value. > > > > > > > > The faulting code is as marked by '-->' below: > > > > > > > > --> if (FixedPcdGet32 (PL011UartInteger) != 0) { > > > > Integer = FixedPcdGet32 (PL011UartInteger); > > > > Fractional = FixedPcdGet32 (PL011UartFractional); > > > > } else { > > > > ... > > > > > > > > The macro FixedPcdGet32 (PL011UartInteger) evaluates > > > > to a macro _PCD_VALUE_PL011UartInteger that is defined > > > > by the build system to represent the UART Integer > > > > value. Therefore, the VS2017 compiler reports the above > > > > warning. > > > > > > > > Fix this warning by enclosing the code in appropriate > > > > #if .. #else .. #endif directives. > > > > > > > > Signed-off-by: Sami Mujawar > > > > > > Reviewed-by: Leif Lindholm > > > > > > > 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. > Now, that might be more cleanly described as a macro than inline like > this - but I think the compiler has usefully warned us about weirdly > written code. So I would really not want to disable the warning on a > global level, and would prefer not to do so here. > > / > Leif > > > > > > > --- > > > > ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c > > > > index 2d3c279cce49304959953ec4a34b50e09a7d0045..dabf099b9bc82e1fb1bd5a2eae3fa4b5878a9e07 100644 > > > > --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c > > > > +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c > > > > @@ -174,10 +174,10 @@ PL011UartInitializePort ( > > > > // > > > > > > > > // If PL011 Integer value has been defined then always ignore the BAUD rate > > > > - if (FixedPcdGet32 (PL011UartInteger) != 0) { > > > > +#if (FixedPcdGet32 (PL011UartInteger) != 0) > > > > Integer = FixedPcdGet32 (PL011UartInteger); > > > > Fractional = FixedPcdGet32 (PL011UartFractional); > > > > - } else { > > > > +#else > > > > // If BAUD rate is zero then replace it with the system default value > > > > if (*BaudRate == 0) { > > > > *BaudRate = FixedPcdGet32 (PcdSerialBaudRate); > > > > @@ -197,7 +197,7 @@ PL011UartInitializePort ( > > > > Divisor = (UINT32)DivisorValue; > > > > Integer = Divisor >> FRACTION_PART_SIZE_IN_BITS; > > > > Fractional = Divisor & FRACTION_PART_MASK; > > > > - } > > > > +#endif > > > > > > > > // > > > > // If PL011 is already initialized, check the current settings > > > > -- > > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > > > >