> -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: 01 December 2017 17:32 > To: Evan Lloyd <[email protected]> > Cc: [email protected]; [email protected]@arm.com > <"[email protected]"@arm.com>; > [email protected]@arm.com <"[email protected]"@arm.com>; > [email protected]@arm.com > <"[email protected]"@arm.com>; [email protected]@arm.com > <"[email protected]"@arm.com> > Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const > qualifier > > On 1 December 2017 at 16:17, Evan Lloyd <[email protected]> wrote: > > Hi Ard. > > Response inline below > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:[email protected]] > >> Sent: 12 October 2017 20:47 > >> To: Evan Lloyd <[email protected]> > >> Cc: [email protected]; "[email protected]"@arm.com; > >> "[email protected]"@arm.com; > >> "[email protected]"@arm.com; "[email protected]"@arm.com > >> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add > const > >> qualifier > >> > >> On 26 September 2017 at 21:15, <[email protected]> wrote: > >> > From: Girish Pathak <[email protected]> > >> > > >> > This change adds some STATIC and CONST qualifiers (mainly to > >> > arguments of functions) in PL111 and HdLcd modules. > >> > > >> > It doesn't add or modify any functionality. > >> > > >> > Contributed-under: TianoCore Contribution Agreement 1.1 > >> > Signed-off-by: Girish Pathak <[email protected]> > >> > Signed-off-by: Evan Lloyd <[email protected]> > >> > --- > >> > > >> > ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr > >> mVExpress.c | 34 ++++++++++---------- > >> > > >> > ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11 > >> 1LcdArmVExpress.c | 34 ++++++++++---------- > >> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c > >> | 4 +-- > >> > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c > >> | 4 +-- > >> > 4 files changed, 38 insertions(+), 38 deletions(-) > >> > > >> > diff --git > >> > > >> > a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> ArmVE > >> > xpress.c > >> > > >> > b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> ArmVE > >> > xpress.c index > >> > > >> > cfe3259d3c737de240350e8c3eab867b80c40948..b9859a56988f7e5be0ad > >> baa49048 > >> > a683fe586bfe 100644 > >> > --- > >> > > >> > a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> ArmVE > >> > xpress.c > >> > +++ > >> > b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd > >> A > >> > +++ rmVExpress.c > >> > @@ -46,7 +46,7 @@ typedef struct { > >> > > >> > /** The display modes supported by the platform. > >> > **/ > >> > -LCD_RESOLUTION mResolutions[] = { > >> > +STATIC CONST LCD_RESOLUTION mResolutions[] = { > >> > { // Mode 0 : VGA : 640 x 480 x 24 bpp > >> > VGA, VGA_H_RES_PIXELS, VGA_V_RES_PIXELS, > >> LCD_BITS_PER_PIXEL_24, > >> > VGA_OSC_FREQUENCY, > >> > @@ -144,8 +144,8 @@ LcdPlatformInitializeDisplay ( **/ EFI_STATUS > >> > LcdPlatformGetVram ( > >> > - OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress, > >> > - OUT UINTN* VramSize > >> > + OUT EFI_PHYSICAL_ADDRESS * CONST VramBaseAddress, > >> > + OUT UINTN * CONST VramSize > >> > >> What is the point of this CONST (and all the other occurrences in > >> this patch) > >> > >> In all cases [AFAICT] the CONST applies to the argument itself, not > >> to the object it points to, which means the variable is CONST in the > >> scope of the function, but can still be dereferenced to assign the OUT > value. > >> > >> This means your change is technically correct, but it is extremely > >> unidiomatic for EDK2, so an explanation why this driver needs this > >> would be highly appreciated. > >> > > [[Evan Lloyd]] The style is explicitly sanctioned by the Edk2 CCS ยง > > 5.6.2.4.2 " Constant pointer to variable: UINTN * CONST ConstPointer; > > ConstPointer is a constant pointer to a variable UINTN." > > > > That paragraph is not about function prototypes, but about constant > pointers in general. > > > The real benefit is that it clearly identifies the pointer as not changed in > the function. > > In this specific instance that also makes it obvious that the OUT > parameters are not array bases, just pointers to individual values. > > > > On a broader note - why would you ever not have a const where > something is not modified? > > > > As another view, the "unidiomatic for EDK2" argument implies you have a > very high opinion of the existing ArmPlatformPkg code quality. > > The "we have always done it that way" argument does not encourage > quality improvements. > > > > This may all be true. But the fact remains that 99% of the EDK2 code does > not constify its function parameters, and I was simply asking why we should > deviate from that in this driver. [[Evan Lloyd]] And the simple answer is that it is good practice, providing cleaner, clearer, safer code. As support for that viewpoint: the CCS references MISRA-C (strangely without making use of the reference, but ...), and MISRA-C has, for example: "Rule 8.13: A pointer should point to a const qualified type where possible"
(NOTE: I don't think it realistic to move edk2 to MISRA-C rules, but they do provide a useful guide to safer practice.) A further point is that it certainly does no harm, and there is negligible benefit and some cost to reducing the quality of tested code. I am also impelled to ask: if 99% of the code is so superb, why are there so many commits from Ard Biesheuvel? (Please note attempt at ironic humour here.) Regards, Evan IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
