Re: [PATCH v2 2/2] ArmPlatformPkg: Fix comparison of constants warning

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

[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é <philmd@redhat.com>
> Sent: 20 May 2020 11:47 AM
> To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; leif@nuviainc.com; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@arm.com>
> 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 <sami.mujawar@arm.com>
>> ---
>>
>> 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.<BR>
>> -  Copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
>>    
>>      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);
>>
>