From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.10841.1589974648397837951 for ; Wed, 20 May 2020 04:37:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YcB2bxUK; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589974647; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gzkbGlJBn+3CtAQBUgkEqONVXWkVg/BBVC10ZzbzVx8=; b=YcB2bxUKFQZmTIKjXDftzlJbqA9MeV9NhDB0df4Wu+5y2TTaK8BuLPjdU+f4sWfEFxW/+0 tAaAFnON1zevoVNioZk0qCcU51hri5J5XrMzdniM1mkAMIQQ7pDAOXdUD5ipdP87zzwYjP ugEx8U2nOD0JdgPC/GkfskiO02VvXSY= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-384-sI5LwH7wMZ-f73YPSgKRLw-1; Wed, 20 May 2020 07:37:23 -0400 X-MC-Unique: sI5LwH7wMZ-f73YPSgKRLw-1 Received: by mail-wm1-f72.google.com with SMTP id p24so1124411wmc.5 for ; Wed, 20 May 2020 04:37:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=gzkbGlJBn+3CtAQBUgkEqONVXWkVg/BBVC10ZzbzVx8=; b=Leq3fqJtC1oPKPo6BvgAHzp3NgoPWd1sFLaHZK3YYm1nZTLgz0YM7dS74m5aKOBo1K ZaHmz3qCDOxqCAKViHzItxgSuNW78NVqFKD9FAXB1lyJnMQn+zpiVnaEIuYr+okjXoe5 y/FEWUAf7hriYct6kbMW+Ax/KnJxsN88FxAkQ8LelgbIdDxqYzAopNnzzA2ABsljL5fi IB5QBdS7QjjETPrWfxf51Lm3LZrqrl9hzMxP+HR1J1IDKILsPZzvni2PM/Gd5BUV0ipu uueQe8G8Qp+WaXsOVJEe47IAWoQW7XcHtRrNKARDIl5bYgOV/DIgZAGuUalSy8fyYmqM uu/Q== X-Gm-Message-State: AOAM531fgq56qDbLW73S9ORmeGgsYdOd5SDdiX7KnjykBkywLgH7/XmG /vBnhzApIx9fENKdbZdD8SPmzIzCyVDQP6/8/On22+QXAQ71sNgipl8sucUVR3dDRmT6GAXRpl6 sL8HAdFMJ91wL0w== X-Received: by 2002:a7b:c24b:: with SMTP id b11mr4232280wmj.101.1589974641745; Wed, 20 May 2020 04:37:21 -0700 (PDT) X-Google-Smtp-Source: ABdhPJySO19MtNHiOzXKPANb+S3Tb5jzb6r5IgiUywouDQMUdK5T/HxdN65rieZEs+A1OFXkUx3atA== X-Received: by 2002:a7b:c24b:: with SMTP id b11mr4232252wmj.101.1589974641496; Wed, 20 May 2020 04:37:21 -0700 (PDT) Return-Path: Received: from [192.168.1.38] (17.red-88-21-202.staticip.rima-tde.net. [88.21.202.17]) by smtp.gmail.com with ESMTPSA id m3sm2524615wrn.96.2020.05.20.04.37.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 May 2020 04:37:20 -0700 (PDT) Subject: Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning To: Sami Mujawar , "devel@edk2.groups.io" Cc: Alexei Fedorov , Ard Biesheuvel , "leif@nuviainc.com" , Matteo Carlini , Laura Moretta , nd References: <20200518124646.45292-1-sami.mujawar@arm.com> <20200518124646.45292-3-sami.mujawar@arm.com> <5eaa5a5c-ace9-b175-a8e9-82d6c2368754@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <18f8bfe7-14d2-3d3d-0e8a-265c3f7a7915@redhat.com> Date: Wed, 20 May 2020 13:37:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit [Top-posting is difficult to read on technical lists; it's better to reply inline; maybe GitHub will fix this problem after all] On 5/20/20 1:25 PM, Sami Mujawar wrote: > Hi Philippe, > > I had put a descriptive message to make people aware that the pragma effects the next line of code only. But I agree we can abbreviate it as per your suggestion. Usually #pragma should be avoided at all cost, but is acceptable as last resort aid. Since you use it in a C source file (and not an header) it is a bit implicit that the scope is local. Sometime too verbose documentation is harmful. Anyway I'm diverging. > However, there is an ongoing discussion about an alternative (by which we could avoid the pragma) on another thread at https://edk2.groups.io/g/devel/message/59948. Good, this approach is preferable. Regards, Phil. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: Philippe Mathieu-Daudé > Sent: 20 May 2020 11:47 AM > To: Sami Mujawar ; devel@edk2.groups.io > Cc: Alexei Fedorov ; Ard Biesheuvel ; leif@nuviainc.com; Matteo Carlini ; Laura Moretta ; nd > Subject: Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning > > Hi Sami, > > On 5/18/20 2:46 PM, 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. >> >> In this case the warning reported by the Visual Studio compiler does >> not evaluate to an issue. However, it can be useful to detect >> potential issues in other scenarios. >> Other compilers may either be incapable of detecting and reporting >> comparison with constant warnings or may be good at reducing false >> positives. So, it is definitely useful to keep this warning enabled, >> and disabling it case by case is a suitable option. >> >> Therefore, disable this warning for Visual studio compilers using the >> pragma suppress directive that: >> 'Pushes the current state of the pragma on the stack, disables the >> specified warning for the next line, and then pops the warning stack >> so that the pragma state is reset.' >> >> Signed-off-by: Sami Mujawar >> --- >> >> Notes: >> v2: >> - Update patch to selectively suppress comparison of [SAMI] >> constant warning and submit as a separate series. >> >> v1: >> - Fix comparison of constant warning reported by VS2017 [SAMI] >> - Various feedbacks can be seen at: >> https://edk2.groups.io/g/devel/topic/32999801#46278 >> >> ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c >> b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c >> index >> 2d3c279cce49304959953ec4a34b50e09a7d0045..3c915e1e8de22a0b0b4cc46d495a >> 5a6cbc784013 100644 >> --- a/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c >> +++ b/ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.c >> @@ -2,7 +2,7 @@ >> Serial I/O Port library functions with no library >> constructor/destructor >> >> Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.
>> - Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.
>> + Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.
>> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> >> @@ -174,6 +174,14 @@ PL011UartInitializePort ( >> // >> >> // If PL011 Integer value has been defined then always ignore the >> BAUD rate >> +#if defined(_MSC_EXTENSIONS) >> + // Suppress 'warning C6326' reported by Visual Studio compiler >> +using >> + // the suppress pragma directive that: 'Pushes the current state of >> + // the pragma on the stack, disables the specified warning for the >> + // next line, and then pops the warning stack so that the pragma >> +state >> + // is reset.' > > We don't need to document how #pragma works each time we use it in the source code... > > What about a simpler comment, referring the particular Visual Studio > version: > > // > > // Disable 'potential comparison of a constant with another constant' > // warning with VS2017 compiler static code analysis option is enabled // > >> +#pragma warning(suppress:6326) >> +#endif >> if (FixedPcdGet32 (PL011UartInteger) != 0) { >> Integer = FixedPcdGet32 (PL011UartInteger); >> Fractional = FixedPcdGet32 (PL011UartFractional); >> >