[edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmCore: Fix compiler warning

Wed Feb 10 08:48:06 UTC 2021

Hi Michael,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki via groups.io
Sent: 10 February 2021 02:59 AM
To: devel at edk2.groups.io; Sami Mujawar <Sami.Mujawar at arm.com>
Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>; Jiewen Yao <jiewen.yao at intel.com>; Supreeth Venkatesh <Supreeth.Venkatesh at arm.com>; nd <nd at arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmCore: Fix compiler warning

Hi Sami,

I'm happy to change the spacing. The code base is very inconsistent with 
this (a somewhat similar scenario would be space before opening 
parenthesis) and it often trends toward a space after the cast. The 
space before opening parenthesis is clearly required. However, I've 
never managed to find a definitive statement in the EDK II C Coding 
Standards Specification regarding typecast spacing.
[SAMI] I agree the code base is inconsistent. However, there are initiatives like the ECC tool that aim to improve this. 
If EDKII core CI is enabled for a Package, it should run the ECC tool. This should prevent new code from introducing inconsistencies i.e., if a check for that rule exists in ECC.
Unfortunately, the EDKII coding standard also has instances of conflicting examples, which I think need improvement.
[/SAMI]

For my own future benefit, could you please point me to the definitive 
statement regarding this rule in the specification?
[SAMI] I agree there is no explicit example for the cast rule in the specification. However, please see point 2 of the horizontal spacing rules at: 
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/v/release%2F2.20/3_quick_reference#3-2-3-formatting-horizontal-spacing
[/SAMI]

In the specification itself, the following section has no space between 
the typecast and the variable:
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/v/release%2F2.20/5_source_files/57_c_programming#5-7-2-3-comparison-of-unsigned-integer-types-to-be-greater-than-0-is-permitted

The following section does have a space between the typecast and variable:
https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/v/release%2F2.20/5_source_files/57_c_programming#5-7-2-4-the-ordering-of-terms-in-predicate-expressions-may-impact-performance-significantly

Thanks,
Michael

On 2/9/2021 2:07 PM, Sami Mujawar wrote:
> Hi Michael,
> 
> Please see my response inline marked [SAMI].
> 
> Other than the minor space change needed to match the coding style, this patch looks good to me.
> 
> With that changed:
> Reviewed-by: Sami Mujawar <sami.mujawar at arm.com>
> 
> Regards,
> 
> Sami Mujawar
> 
> -----Original Message-----
> From: mikuback at linux.microsoft.com <mikuback at linux.microsoft.com>
> Sent: 03 February 2021 03:51 AM
> To: devel at edk2.groups.io
> Cc: Ard Biesheuvel <Ard.Biesheuvel at arm.com>; Sami Mujawar <Sami.Mujawar at arm.com>; Jiewen Yao <jiewen.yao at intel.com>; Supreeth Venkatesh <Supreeth.Venkatesh at arm.com>
> Subject: [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmCore: Fix compiler warning
> 
> From: Michael Kubacki <michael.kubacki at microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3204
> 
> Fixes the following compiler warning in VS2019 by changing defining
> the MmramRangeCount variable to be UINTN and type casting prior
> to value assignment.
> 
> \edk2\StandaloneMmPkg\Core\StandaloneMmCore.c(570): error C2220:
>    the following warning is treated as an error
> \edk2\StandaloneMmPkg\Core\StandaloneMmCore.c(570): warning C4244:
>    '=': conversion from 'UINT64' to 'UINT32', possible loss of data
> 
> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> Cc: Sami Mujawar <sami.mujawar at arm.com>
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Supreeth Venkatesh <supreeth.venkatesh at arm.com>
> Signed-off-by: Michael Kubacki <michael.kubacki at microsoft.com>
> ---
>   StandaloneMmPkg/Core/StandaloneMmCore.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Core/StandaloneMmCore.c
> index 8388ec289ca8..d254a68f2fb8 100644
> --- a/StandaloneMmPkg/Core/StandaloneMmCore.c
> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
> @@ -511,7 +511,7 @@ StandaloneMmMain (
>     EFI_HOB_GUID_TYPE               *MmramRangesHob;
>     EFI_MMRAM_HOB_DESCRIPTOR_BLOCK  *MmramRangesHobData;
>     EFI_MMRAM_DESCRIPTOR            *MmramRanges;
> -  UINT32                          MmramRangeCount;
> +  UINTN                           MmramRangeCount;
>     EFI_HOB_FIRMWARE_VOLUME         *BfvHob;
>   
>     ProcessLibraryConstructorList (HobStart, &gMmCoreMmst);
> @@ -546,7 +546,7 @@ StandaloneMmMain (
>       MmramRangesHobData = GET_GUID_HOB_DATA (MmramRangesHob);
>       ASSERT (MmramRangesHobData != NULL);
>       MmramRanges = MmramRangesHobData->Descriptor;
> -    MmramRangeCount = MmramRangesHobData->NumberOfMmReservedRegions;
> +    MmramRangeCount = (UINTN) MmramRangesHobData->NumberOfMmReservedRegions;
> [SAMI] There should be no space between the typecast and the variable, i.e. space after typecast (UINTN) and MmramRangesHobData.
> Same at other places in this file.
> [/SAMI]
> 
>       ASSERT (MmramRanges);
>       ASSERT (MmramRangeCount);
>   
> @@ -554,7 +554,7 @@ StandaloneMmMain (
>       // Copy the MMRAM ranges into MM_CORE_PRIVATE_DATA table just in case any
>       // code relies on them being present there
>       //
> -    gMmCorePrivate->MmramRangeCount = MmramRangeCount;
> +    gMmCorePrivate->MmramRangeCount = (UINT64) MmramRangeCount;
>       gMmCorePrivate->MmramRanges =
>         (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (MmramRangeCount * sizeof (EFI_MMRAM_DESCRIPTOR));
>       ASSERT (gMmCorePrivate->MmramRanges != 0);
> @@ -567,7 +567,7 @@ StandaloneMmMain (
>       DataInHob       = GET_GUID_HOB_DATA (GuidHob);
>       gMmCorePrivate = (MM_CORE_PRIVATE_DATA *)(UINTN)DataInHob->Address;
>       MmramRanges     = (EFI_MMRAM_DESCRIPTOR *)(UINTN)gMmCorePrivate->MmramRanges;
> -    MmramRangeCount = gMmCorePrivate->MmramRangeCount;
> +    MmramRangeCount = (UINTN) gMmCorePrivate->MmramRangeCount;
>     }
>   
>     //
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71571): https://edk2.groups.io/g/devel/message/71571
Mute This Topic: https://groups.io/mt/80346365/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-