Patchwork [2/3] flash_eraseall: move constants out of for loop

login
register
mail settings
Submitter Shevchenko Andriy (EXT-Teleca/Helsinki)
Date June 17, 2010, 8:42 a.m.
Message ID <1276764159-28726-2-git-send-email-ext-andriy.shevchenko@nokia.com>
Download mbox | patch
Permalink /patch/56405/
State New
Headers show

Comments

Shevchenko Andriy (EXT-Teleca/Helsinki) - June 17, 2010, 8:42 a.m.
Signed-off-by: Andy Shevchenko <ext-andriy.shevchenko@nokia.com>
---
 flash_eraseall.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)
Mike Frysinger - June 22, 2010, 5:33 p.m.
On Thu, Jun 17, 2010 at 04:42, Shevchenko Andriy wrote:
> --- a/flash_eraseall.c
> +++ b/flash_eraseall.c
> @@ -194,21 +194,21 @@ void process_options (int argc, char *argv[])
>  {
>        int error = 0;
>
> +       static const char *short_options = "jq";
> +       static const struct option long_options[] = {
> +               {"help", no_argument, 0, 0},
> +               {"version", no_argument, 0, 0},
> +               {"jffs2", no_argument, 0, 'j'},
> +               {"quiet", no_argument, 0, 'q'},
> +               {"silent", no_argument, 0, 'q'},
> +
> +               {0, 0, 0, 0},
> +       };
> +
>        exe_name = argv[0];
>
>        for (;;) {
>                int option_index = 0;
> -               static const char *short_options = "jq";
> -               static const struct option long_options[] = {
> -                       {"help", no_argument, 0, 0},
> -                       {"version", no_argument, 0, 0},
> -                       {"jffs2", no_argument, 0, 'j'},
> -                       {"quiet", no_argument, 0, 'q'},
> -                       {"silent", no_argument, 0, 'q'},
> -
> -                       {0, 0, 0, 0},
> -               };
> -
>                int c = getopt_long(argc, argv, short_options,
>                                long_options, &option_index);
>                if (c == EOF) {

NAK: no explanation why you're doing this, and current code has the
variables scoped to where they actually get used.

i wonder though why this code even bothers with "static".
-mike
Jamie Lokier - June 23, 2010, 12:23 a.m.
Mike Frysinger wrote:
> NAK: no explanation why you're doing this, and current code has the
> variables scoped to where they actually get used.
> 
> i wonder though why this code even bothers with "static".

The array is static to avoid compiling to code which fills in the
array at runtime.  I.e. it makes the code smaller, to the same size as
if they were globals.  And then, only because its static, the const
can put them in the .rodata section, reducing unshared data size.

Because they're static there's no benefit to moving them to another
scope.

-- Jamie
Mike Frysinger - June 23, 2010, 2:11 a.m.
On Tue, Jun 22, 2010 at 20:23, Jamie Lokier wrote:
> Mike Frysinger wrote:
>> i wonder though why this code even bothers with "static".
>
> The array is static to avoid compiling to code which fills in the
> array at runtime.  I.e. it makes the code smaller, to the same size as
> if they were globals.  And then, only because its static, the const
> can put them in the .rodata section, reducing unshared data size.

if it were generated on the stack at runtime, the .text is shared too

> Because they're static there's no benefit to moving them to another
> scope.

that's sort of what i expected, but i found it odd that it isnt:
    static const char * const short_options = "jq";
-mike
Jamie Lokier - June 24, 2010, 12:39 a.m.
Mike Frysinger wrote:
> On Tue, Jun 22, 2010 at 20:23, Jamie Lokier wrote:
> > Mike Frysinger wrote:
> >> i wonder though why this code even bothers with "static".
> >
> > The array is static to avoid compiling to code which fills in the
> > array at runtime.  I.e. it makes the code smaller, to the same size as
> > if they were globals.  And then, only because its static, the const
> > can put them in the .rodata section, reducing unshared data size.
> 
> if it were generated on the stack at runtime, the .text is shared too

Shared, but larger than the static-const data it replaces.

> > Because they're static there's no benefit to moving them to another
> > scope.
> 
> that's sort of what i expected, but i found it odd that it isnt:
>     static const char * const short_options = "jq";

You're right, I missed that.
I'd have used this myself:

static const char short_options[] = "jq";

-- Jamie

Patch

diff --git a/flash_eraseall.c b/flash_eraseall.c
index a22fc49..f6319d1 100644
--- a/flash_eraseall.c
+++ b/flash_eraseall.c
@@ -194,21 +194,21 @@  void process_options (int argc, char *argv[])
 {
 	int error = 0;
 
+	static const char *short_options = "jq";
+	static const struct option long_options[] = {
+		{"help", no_argument, 0, 0},
+		{"version", no_argument, 0, 0},
+		{"jffs2", no_argument, 0, 'j'},
+		{"quiet", no_argument, 0, 'q'},
+		{"silent", no_argument, 0, 'q'},
+
+		{0, 0, 0, 0},
+	};
+
 	exe_name = argv[0];
 
 	for (;;) {
 		int option_index = 0;
-		static const char *short_options = "jq";
-		static const struct option long_options[] = {
-			{"help", no_argument, 0, 0},
-			{"version", no_argument, 0, 0},
-			{"jffs2", no_argument, 0, 'j'},
-			{"quiet", no_argument, 0, 'q'},
-			{"silent", no_argument, 0, 'q'},
-
-			{0, 0, 0, 0},
-		};
-
 		int c = getopt_long(argc, argv, short_options,
 				long_options, &option_index);
 		if (c == EOF) {