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. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
