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:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (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 EDC9920352A84 for ; Fri, 1 Dec 2017 09:27:09 -0800 (PST) Received: by mail-io0-x242.google.com with SMTP id g1so12002668ioc.8 for ; Fri, 01 Dec 2017 09:31:36 -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=NZRBUbDy/VAibDDFC0TYLOqQhm6dlcn8vUKAc1Jgp0E=; b=EwvF+uThxm99YPCxG1SUCUdd3YYwE9dupMaqFE2nDP5k8wprlgVJZjgiSTlLAnss6D myLEAinMgFPG/XnIY0ZmRLu8uXKtwbB8PgUtoJgB4/O4TQ2loH2pPUD82HxhaE3rvAch iOXpfkyYgKn3wBO9nEOtYLAKqSwX9rcNeV9AM= 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=NZRBUbDy/VAibDDFC0TYLOqQhm6dlcn8vUKAc1Jgp0E=; b=NGXWV3ZTclmHLzNbl2SQazxUC3YK3sVyAsNqs9t6AswvreCrrO5FZ0XXcn+b3bXbTP RPXknZiB25mCJqA7Nyt/9qbEhnFdZDp4zhV5kWw8MeWAk1UGkZO4O53gLwhGkjQiL5In K+ENgGQLrs77vrdGbSK09CHvgeMQFLNZr7D02EizyPBimH09t7vC3FWKm5phHnc/n/Ys 4YtPSq/2fzlR3dcrUTn3+Lg5a17kyYHXjGgV9N3N4Pu7YSpZppLSm/V3fyKpRzNoIT5F qfuZ8y/ZLmjeleCnsh9OQX5BNC6qnGYJEyFr3tE21A8dduHiUKPG5qm3yt7CI/YojkpG zRHQ== X-Gm-Message-State: AJaThX7dN9C5FwIO+lku5ys4+eKYp3iRXbPsjnpntFL6rBys44sWgPG3 x/+wn/TzVXEHE5S4raiucxGUnI0tlS9qWKu+Jp1tkQ== X-Google-Smtp-Source: AGs4zMZTpzCjEqoRcX1RLCYPN2WRl9nr+4RcFIBhoazouiRAiZRCcR5/mdCA7gjBc0vwZviKn9Y7DXdV9WO13G3x5rw= X-Received: by 10.107.178.145 with SMTP id b139mr13509451iof.52.1512149495776; Fri, 01 Dec 2017 09:31:35 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Fri, 1 Dec 2017 09:31:35 -0800 (PST) In-Reply-To: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-4-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Fri, 1 Dec 2017 17:31:35 +0000 Message-ID: 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 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: Fri, 01 Dec 2017 17:27:10 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 p= atch) >> >> In all cases [AFAICT] the CONST applies to the argument itself, not to t= he >> object it points to, which means the variable is CONST in the scope of t= he >> 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 =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 changed= in the function. > In this specific instance that also makes it obvious that the OUT paramet= ers are not array bases, just pointers to individual values. > > On a broader note - why would you ever not have a const where something i= s not modified? > > As another view, the "unidiomatic for EDK2" argument implies you have a v= ery high opinion of the existing ArmPlatformPkg code quality. > The "we have always done it that way" argument does not encourage qualit= y 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.