On 5 December 2017 at 20:35, Evan Lloyd <[email protected]> wrote: > > >> -----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" >
Should *point to* which means UINTN CONST *Var not UINTN * CONST Var as you are proposing, so this reference is completely irrelevant. > (NOTE: I don't think it realistic to move edk2 to MISRA-C rules, but they do > provide a useful guide to safer practice.) > Well, for the firmware in my ABS brake system, yes, but for higher level stuff it is overly restrictive imo. > 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 have read that sentence a couple of times, and I don't understand what you are trying to say here - sorry. Could you rephrase that? > 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.) > No amount of drugs or alcohol could ever elicit such a statement from me. I was merely pointing out that constifying function arguments is very uncommon, in any project I am involved in. (EDK2, Linux, Qemu) I agree that it may help catch programming errors, but beyond that, I think the value is limited. I won't object to new code that does it, but changing existing code is just churn imo. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
