threads / patch / 7657

patchCorrect dir.c to compile on Solaris 9

Subject: [PATCH] Correct dir.c to compile on Solaris 9

## tl;dr

5 messages between Apr 15, 2007 and Apr 15, 2007. Diffs are folded; open one to read it.

replies: 4people: 5as markdown or json

Shawn O. Pearce· Apr 15, 2007, 04:33 UTC · lore

The compiler on my Solaris 9 system doesn't understand the array initialization syntax used here in dir.c.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 dir.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

Show changes to dir.c +5 −6

diff --git a/dir.c b/dir.c
index 7426fde..038fd82 100644
--- a/dir.c
+++ b/dir.c
@@ -423,18 +423,17 @@ static int cmp_name(const void *p1, const void *p2)
  */
 static int simple_length(const char *match)
 {
-	const char special[256] = {
-		[0] = 1, ['?'] = 1,
-		['\\'] = 1, ['*'] = 1,
-		['['] = 1
-	};
 	int len = -1;
 
 	for (;;) {
 		unsigned char c = *match++;
 		len++;
-		if (special[c])
+		switch (c) {
+		case 0: case '?':
+		case '\\': case '*':
+		case '[':
 			return len;
+		}
 	}
 }

-- 
1.5.1.1.83.g2bfe3

Johannes Schindelin· Apr 15, 2007, 16:25 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Correct dir.c to compile on Solaris 9

Hi,

On Sun, 15 Apr 2007, Shawn O. Pearce wrote:

Show 35 quoted lines

> The compiler on my Solaris 9 system doesn't understand
> the array initialization syntax used here in dir.c.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  dir.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/dir.c b/dir.c
> index 7426fde..038fd82 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -423,18 +423,17 @@ static int cmp_name(const void *p1, const void *p2)
>   */
>  static int simple_length(const char *match)
>  {
> -	const char special[256] = {
> -		[0] = 1, ['?'] = 1,
> -		['\\'] = 1, ['*'] = 1,
> -		['['] = 1
> -	};
>  	int len = -1;
>  
>  	for (;;) {
>  		unsigned char c = *match++;
>  		len++;
> -		if (special[c])
> +		switch (c) {
> +		case 0: case '?':
> +		case '\\': case '*':
> +		case '[':
>  			return len;
> +		}
>  	}
>  }

You are replacing a table-based check with a switch based, which might be substantially slower (depends on how often cmp_name() is called).

Maybe there is another way to initialize the table (and make it static to begin with)?

Ciao, Dscho

Josef Weidendorfer· Apr 15, 2007, 21:03 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Correct dir.c to compile on Solaris 9

On Sunday 15 April 2007, Johannes Schindelin wrote:

Show 25 quoted lines

> On Sun, 15 Apr 2007, Shawn O. Pearce wrote:
> >  static int simple_length(const char *match)
> >  {
> > -	const char special[256] = {
> > -		[0] = 1, ['?'] = 1,
> > -		['\\'] = 1, ['*'] = 1,
> > -		['['] = 1
> > -	};
> >  	int len = -1;
> >  
> >  	for (;;) {
> >  		unsigned char c = *match++;
> >  		len++;
> > -		if (special[c])
> > +		switch (c) {
> > +		case 0: case '?':
> > +		case '\\': case '*':
> > +		case '[':
> >  			return len;
> > +		}
> >  	}
> >  }
> 
> You are replacing a table-based check with a switch based, which might be 
> substantially slower (depends on how often cmp_name() is called).

Or faster. When the table gives a cache miss and has to be loaded from main memory, I am quite sure that 5 compares in a row are faster than the cache miss.

Actually, with the switch, the compiler is free to implement it with a table (and gcc usually does this, probably even using a substantially smaller table). The table-based check in contrast looks like some kind of micro-optimization which makes the code IMHO more difficult to read, and which only would be justified with meassured improvements.

Josef

Robin Rosenberg· Apr 15, 2007, 21:54 UTC · re: Josef Weidendorfer · lore

Re: [PATCH] Correct dir.c to compile on Solaris 9

söndag 15 april 2007 23:03 skrev Josef Weidendorfer:

Show 30 quoted lines

> On Sunday 15 April 2007, Johannes Schindelin wrote:
> > On Sun, 15 Apr 2007, Shawn O. Pearce wrote:
> > >  static int simple_length(const char *match)
> > >  {
> > > -	const char special[256] = {
> > > -		[0] = 1, ['?'] = 1,
> > > -		['\\'] = 1, ['*'] = 1,
> > > -		['['] = 1
> > > -	};
> > >  	int len = -1;
> > >  
> > >  	for (;;) {
> > >  		unsigned char c = *match++;
> > >  		len++;
> > > -		if (special[c])
> > > +		switch (c) {
> > > +		case 0: case '?':
> > > +		case '\\': case '*':
> > > +		case '[':
> > >  			return len;
> > > +		}
> > >  	}
> > >  }
> > 
> > You are replacing a table-based check with a switch based, which might be 
> > substantially slower (depends on how often cmp_name() is called).
> 
> Or faster. When the table gives a cache miss and has to be
> loaded from main memory, I am quite sure that 5 compares in a row are
> faster than the cache miss.

It is five compares time the length of the path being examined. The cache miss only occurs once per invocation even inthe worst case. Judging from where the code is invoked, cache misses should be rare which means the table is much faster.

As for the table being a micro-optimization, that still holds true

> 
> Actually, with the switch, the compiler is free to implement it with a
> table (and gcc usually does this, probably even using a substantially
> smaller table). The table-based check in contrast looks

It usually uses binary search. For gcc to create a lookup table you'll need a large number of case's.

> like some kind of micro-optimization which makes the code IMHO more
> difficult to read, and which only would be justified with meassured
> improvements.

The table lookup *is* faster (meastured), but that doesn't make a big difference on the total CPU used. The muliple-case-per line thing (both versions, however makes is hard to read.

-- robin

Show changes to dir.c +15 −5

diff --git a/dir.c b/dir.c
index 7426fde..0780f23 100644
--- a/dir.c
+++ b/dir.c
@@ -423,12 +423,22 @@ static int cmp_name(const void *p1, const void *p2)
  */
 static int simple_length(const char *match)
 {
-       const char special[256] = {
-               [0] = 1, ['?'] = 1,
-               ['\\'] = 1, ['*'] = 1,
-               ['['] = 1
-       };
        int len = -1;
+       static const char special[256] = {
+               1,0,0,0,0,0,0,0, /* nul */
+               0,0,0,0,0,0,0,0,
+               0,0,0,0,0,0,0,0,
+               0,0,0,0,0,0,0,0,
+               0,0,0,0,0,0,0,0,
+               0,0,1,0,0,0,0,0, /* * */
+               0,0,0,0,0,0,0,0,
+               0,0,0,0,0,0,0,1, /* ? */
+
+               0,0,0,0,0,0,0,0,
+               0,0,0,0,0,0,0,0,
+               0,0,0,0,0,0,0,0,
+               0,0,0,1,1,0,0,0  /* [ \ */
+       };

        for (;;) {
                unsigned char c = *match++;

Junio C Hamano· Apr 15, 2007, 22:48 UTC · re: Robin Rosenberg · lore

Re: [PATCH] Correct dir.c to compile on Solaris 9

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

Show 27 quoted lines

> The table lookup *is* faster (meastured), but that doesn't make a big difference
> on the total CPU used. The muliple-case-per line thing (both versions, however makes is hard
> to read.  
>
> -- robin
>
> diff --git a/dir.c b/dir.c
> index 7426fde..0780f23 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -423,12 +423,22 @@ static int cmp_name(const void *p1, const void *p2)
>   */
>  static int simple_length(const char *match)
>  {
> -       const char special[256] = {
> -               [0] = 1, ['?'] = 1,
> -               ['\\'] = 1, ['*'] = 1,
> -               ['['] = 1
> -       };
>         int len = -1;
> +       static const char special[256] = {
> +               1,0,0,0,0,0,0,0, /* nul */
> +               0,0,0,0,0,0,0,0,
> +               0,0,0,0,0,0,0,0,
> +               0,0,0,0,0,0,0,0,
> +               0,0,0,0,0,0,0,0,
> +               0,0,1,0,0,0,0,0, /* * */

I wonder if folding this into ctype.c::sane_ctype[] is an option...