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.web09.23340.1574340800197753018 for ; Thu, 21 Nov 2019 04:53:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=EvTIPXct; spf=pass (domain: linaro.org, ip: 209.85.128.66, mailfrom: leif.lindholm@linaro.org) Received: by mail-wm1-f66.google.com with SMTP id f129so2228032wmf.2 for ; Thu, 21 Nov 2019 04:53:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Tn8ws3th1czo9smkqgaRCugv3ABwEaQULsfAiE/do9s=; b=EvTIPXctFu0C9EKZvvyOaDP0ZUFcq1nGik9oSm2a9ShC6Nc3IaLXCKOR+eVgXiqmkz O7yMY5H6CVfJqdPDkP0ffJP1BiHyxnyFWXOo14VyPOeYRbNdsZZGwSFtsqwaeMMzNjgS RUIX5Acadr+HxZAaMQMpH0AlBugGf6u4BUseH1RU6kLMcvkI8RvaFK0EC094PhD5UiQB NVLK/CAi0NpN86UK0BGTQnzvXETK+JDMFOicc1PfXxFQ9hfQQcysmpVnH+SJgW+0WrW7 sh//IhjjZE3Cfc6xHgxT/tn6EqsJbIpyaJP9InRdvI+eC8LlgKp+xO+oZWAc/V0CgBjA H3VQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Tn8ws3th1czo9smkqgaRCugv3ABwEaQULsfAiE/do9s=; b=epp9ejgkHAiMZX04iok6ThkAADuGrlhFvBLOkpnlzoUfs+Ww4zFi9dClk9waEblzfF p7qkjVCpEpHCyqWciJizB2KOo2tIjo20k5wrCPsxurM7L7GuPsHWNNI5vg0rzfMsvdQ+ DdKl3gzYlWkGTgcJYg5Qpih2CyfLW5OFLMCO3C1/+92f2QU37id2du775Qb9rDgsWJ55 QnzP+bRDuxQvsUvr5uab8n+3kc+FHNR1drDX7DW/g2PJln6B0h8P4ktSc9HSvJQYt6bQ WcpmP8hKl4p56nirI3FNHrZR9QItG2+gea5LAAE0jrrTSuTtWO58aP3lJ+8SgbDOs5hH MtXg== X-Gm-Message-State: APjAAAVznoVPHtt7rgGdIoFQbtzdfDpc71hR3UjaHZ9y/BTsAkL0r+Jq JrJk9dIlh/jD5J5DCedyd+KqVQ== X-Google-Smtp-Source: APXvYqzj/pAq6bd+vbRQNLCi8GdOaPaBYfyzOl8hxj/ob+cEmmuM64BA8rYJES+0DJguuKLsdk+teA== X-Received: by 2002:a1c:9986:: with SMTP id b128mr2653038wme.154.1574340798595; Thu, 21 Nov 2019 04:53:18 -0800 (PST) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 17sm2720046wmg.19.2019.11.21.04.53.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Nov 2019 04:53:17 -0800 (PST) Date: Thu, 21 Nov 2019 12:53:15 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: Sami Mujawar , edk2-devel-groups-io , Alexei Fedorov , Matteo Carlini , nd Subject: Re: [PATCH v1 18/19] ArmPlatformPkg: Fix comparison of constants warning Message-ID: <20191121125315.GF7359@bivouac.eciton.net> References: <20190823105539.13260-1-sami.mujawar@arm.com> <20190823105539.13260-19-sami.mujawar@arm.com> <20191121123542.GD7359@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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?". 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)' > > > > > >