From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22b; helo=mail-it0-x22b.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22b.google.com (mail-it0-x22b.google.com [IPv6:2607:f8b0:4001:c0b::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 499A62218E93F for ; Tue, 5 Dec 2017 12:49:52 -0800 (PST) Received: by mail-it0-x22b.google.com with SMTP id o130so18366123itg.0 for ; Tue, 05 Dec 2017 12:54:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=RoyBBSJdLQFNANZKRnzsNlz6TeW/m/rcPebnvCDYAKk=; b=HrDnFRvkjFwl4+/w7o6oNEBm89W4d4k8/kbLssYrLLPA650TQRWeh6D9xXSMSFK3p/ 4ahhNWU+CIqsTDa+LPhMKhyJjeftt2lsVqa4PaV4cIcPDyLeFS27wWYES/YDWsh9Cg0K vLOL/Vicamp5cxPpYuhN2q6jE29gjn27sOT80= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=RoyBBSJdLQFNANZKRnzsNlz6TeW/m/rcPebnvCDYAKk=; b=M8AFEIVVeDUef4ltCPQZXP+pi+Gs0xfl3iQm9R7BBzTlPNWG4yRKs056LMPMefULA0 FApilr0XemDUSvzjVbBygGyzQ0lVZ4rKYxJVa4vBJBOjlLz/aLlGr9Mlagr1f+4/NlNV pIR55sfEr5eL0gExWUvzf71RJzFAvbQfev8RWIplOMZly/dVlfwI1S7iiX4gq0yHBfL3 XWQce3FTF02Rv4C3WrbWSE5/Fp35z2ZMKQwfbzPmdRNiOoWQIzCQIbXJ+vlHYkmuRoxc HfYm453q58NFnD/cy81ipQ0uIx7271MHS9rGDaRCnTrAukW69vw3zAj5cAlJb4NRWeaq RK9Q== X-Gm-Message-State: AKGB3mJxJG1dFjPjwLAZ7IU4XHzVGAZRnI15kPMO6YDbHH6M3iBSuDMO LcesyCGXzXte+kEffUpQ8A+0RUgDyI1/tsOOzEnfkQ== X-Google-Smtp-Source: AGs4zMamoS/Xxlf+zQmQ+mlRbiPRh/YL3U0Kaa6yuQRpgjJKWiqHTSMKR3s0bATYH93xvl+TsSggRb1iPniqx5IiER8= X-Received: by 10.36.145.203 with SMTP id i194mr10898106ite.73.1512507263382; Tue, 05 Dec 2017 12:54:23 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Tue, 5 Dec 2017 12:54:22 -0800 (PST) In-Reply-To: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-4-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Tue, 5 Dec 2017 20:54:22 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Matteo Carlini Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const qualifier X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Dec 2017 20:49:53 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 5 December 2017 at 20:35, Evan Lloyd wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 01 December 2017 17:32 >> To: Evan Lloyd >> Cc: edk2-devel@lists.01.org; ard.biesheuvel@linaro.org@arm.com >> <"ard.biesheuvel@linaro.org"@arm.com>; >> leif.lindholm@linaro.org@arm.com <"leif.lindholm@linaro.org"@arm.com>; >> Matteo.Carlini@arm.com@arm.com >> <"Matteo.Carlini@arm.com"@arm.com>; nd@arm.com@arm.com >> <"nd@arm.com"@arm.com> >> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const >> qualifier >> >> On 1 December 2017 at 16:17, Evan Lloyd wrote: >> > Hi Ard. >> > Response inline below >> > >> >> -----Original Message----- >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> >> Sent: 12 October 2017 20:47 >> >> To: Evan Lloyd >> >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com; >> >> "leif.lindholm@linaro.org"@arm.com; >> >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com >> >> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add >> const >> >> qualifier >> >> >> >> On 26 September 2017 at 21:15, wrote: >> >> > From: Girish Pathak >> >> > >> >> > 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 >> >> > Signed-off-by: Evan Lloyd >> >> > --- >> >> > >> >> >> 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[] =3D { >> >> > +STATIC CONST LCD_RESOLUTION mResolutions[] =3D { >> >> > { // 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 OU= T >> 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 =C2= =A7 >> > 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 chan= ged 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 doe= s >> not constify its function parameters, and I was simply asking why we sho= uld >> deviate from that in this driver. > [[Evan Lloyd]] And the simple answer is that it is good practice, providi= ng cleaner, clearer, safer code. As support for that viewpoint: the CCS re= ferences 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 possib= le" > 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 negligibl= e 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 humou= r 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.