public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Andrew Fish <afish@apple.com>
To: Kurt Kennett <Kurt.Kennett@microsoft.com>
Cc: Dandan Bi <dandan.bi@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Ruiyu Ni <ruiyu.ni@intel.com>
Subject: Re: [patch 2/8] FatPkg\EnhancedFatDxe: Initialize variable after declaration
Date: Thu, 08 Dec 2016 15:11:12 -0800	[thread overview]
Message-ID: <D9EA7BBB-2800-4804-8527-8BBDEF89958E@apple.com> (raw)
In-Reply-To: <CY1PR03MB234553D120C334AB24573F4A9C840@CY1PR03MB2345.namprd03.prod.outlook.com>


> On Dec 8, 2016, at 9:27 AM, Kurt Kennett <Kurt.Kennett@microsoft.com> wrote:
> 
> This seems kind of silly.
> Why isn't this just const data?  This adds code and memory accesses that are worthless and happen on every call to the function.
> 

K2,

I think this is an attempt to conform to the coding standard?

I do agree it is pointless to make it static if you are going to initialize it every time. Seems like it would have been better to make this a global variable. 

Given that a static local variable is very much like a global that is limited in scope it may make sense to relax the coding standard in this regard (I did not re-read the coding standard so hopefully this is not a bad assumption), or just require pre-initialized local static variables to be globals?


In this simple example you can see that a local static variable that is initialized ends up with code generation that is very close to a global variable. The name is not global and it is mangled with the function name. The other interesting tidbit is the compiler realized the static was const and marked it as such. 

~/work/Compiler>cat static.c
const unsigned char gMonthDays[] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 131 };

int test (int i)
{
  static unsigned char MonthDays[] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };

  return MonthDays[i];
}
~/work/Compiler>clang -S -Os static.c
~/work/Compiler>cat static.S
	.section	__TEXT,__text,regular,pure_instructions
	.globl	_test
_test:                                  ## @test
	pushq	%rbp
	movq	%rsp, %rbp
	movslq	%edi, %rax
	leaq	_test.MonthDays(%rip), %rcx
	movzbl	(%rax,%rcx), %eax
	popq	%rbp
	retq

	.section	__TEXT,__const
	.globl	_gMonthDays             ## @gMonthDays
_gMonthDays:
	.ascii	"\037\034\037\036\037\036\037\037\036\037\036\203"

_test.MonthDays:                        ## @test.MonthDays
	.ascii	"\037\034\037\036\037\036\037\037\036\037\036\037"

Note: I had to make gMonthDays different than _test.MonthDays or _test.MonthDays gets optimized out. 

Thanks,

Andrew Fish


> K2
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Dandan Bi
> Sent: Thursday, December 8, 2016 2:54 AM
> To: edk2-devel@lists.01.org
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Subject: [edk2] [patch 2/8] FatPkg\EnhancedFatDxe: Initialize variable after declaration
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
> FatPkg/EnhancedFatDxe/Misc.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/FatPkg/EnhancedFatDxe/Misc.c b/FatPkg/EnhancedFatDxe/Misc.c index f91759c..6ad688c 100644
> --- a/FatPkg/EnhancedFatDxe/Misc.c
> +++ b/FatPkg/EnhancedFatDxe/Misc.c
> @@ -696,15 +696,27 @@ Returns:
>   TRUE                  - The time is valid.
>   FALSE                 - The time is not valid.
> 
> --*/
> {
> -  static UINT8  MonthDays[] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
> +  STATIC UINT8  MonthDays[12];
>   UINTN         Day;
>   BOOLEAN       ValidTime;
> 
>   ValidTime = TRUE;
> +  MonthDays[0] = 31;
> +  MonthDays[1] = 28;
> +  MonthDays[2] = 31;
> +  MonthDays[3] = 30;
> +  MonthDays[4] = 31;
> +  MonthDays[5] = 30;
> +  MonthDays[6] = 31;
> +  MonthDays[7] = 31;
> +  MonthDays[8] = 30;
> +  MonthDays[9] = 31;
> +  MonthDays[10] = 30;
> +  MonthDays[11] = 31;
> 
>   //
>   // Check the fields for range problems
>   // Fat can only support from 1980
>   //
> --
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel



  reply	other threads:[~2016-12-08 23:11 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-08 10:54 [patch 0/8] FatPkg: Fix coding style issues Dandan Bi
2016-12-08 10:54 ` [patch 1/8] FatPkg\EnhancedFatDxe: Avoid Non-Boolean type uses as Boolean Dandan Bi
2016-12-09  1:14   ` Ni, Ruiyu
2016-12-08 10:54 ` [patch 2/8] FatPkg\EnhancedFatDxe: Initialize variable after declaration Dandan Bi
2016-12-08 17:27   ` Kurt Kennett
2016-12-08 23:11     ` Andrew Fish [this message]
2016-12-20 23:26       ` Brian J. Johnson
2016-12-08 23:47     ` Yao, Jiewen
2016-12-09  0:26       ` Kurt Kennett
2016-12-09  0:36         ` Andrew Fish
2016-12-09  0:40           ` Yao, Jiewen
2016-12-09 16:32             ` Kurt Kennett
2016-12-09  0:40           ` Kurt Kennett
2016-12-09  0:57   ` Ni, Ruiyu
2016-12-09  1:18     ` Bi, Dandan
2016-12-08 10:54 ` [patch 3/8] FatPkg\EnhancedFatDxe: Make function prototype align with definition Dandan Bi
2016-12-09  1:13   ` Ni, Ruiyu
2016-12-08 10:54 ` [patch 4/8] FatPkg\EnhancedFatDxe: Make the variable name follow rule Dandan Bi
2016-12-09  1:12   ` Ni, Ruiyu
2016-12-08 10:54 ` [patch 5/8] FatPkg\EnhancedFatDxe: Use typedef for complex type Dandan Bi
2016-12-09  1:11   ` Ni, Ruiyu
2016-12-08 10:54 ` [patch 6/8] FatPkg\EnhancedFatDxe: Make the comments align with EDKII coding style Dandan Bi
2016-12-09  1:10   ` Ni, Ruiyu
2016-12-08 10:54 ` [patch 7/8] FatPkg\EnhancedFatDxe: Add comments for functions Dandan Bi
2016-12-09  1:10   ` Ni, Ruiyu
2016-12-08 10:54 ` [patch 8/8] FatPkg: Fix format issues in dec/inf/dsc files Dandan Bi
2016-12-09  1:09   ` Ni, Ruiyu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D9EA7BBB-2800-4804-8527-8BBDEF89958E@apple.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox