[PATCH] Fix Solaris compiler warnings

[PATCH] Fix Solaris compiler warnings

[Guido Ostkamp]

Hello,

the below patch fixes some compiler warnings returned by Solaris Workshop 
Compilers.

     CC builtin-apply.o
"builtin-apply.c", line 686: warning: statement not reached
     CC utf8.o
"utf8.c", line 287: warning: statement not reached
     CC xdiff/xdiffi.o
"xdiff/xdiffi.c", line 261: warning: statement not reached
     CC xdiff/xutils.o
"xdiff/xutils.c", line 236: warning: statement not reached

Signed-off-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
---
  builtin-apply.c |    1 -
  utf8.c          |    1 -
  xdiff/xdiffi.c  |    2 --
  xdiff/xutils.c  |    2 --
  4 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 8edcc08..91f8752 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -683,7 +683,6 @@ static char *git_header_name(char *line, int llen)
  			}
  		}
  	}
-	return NULL;
  }

  /* Verify that we recognize the lines following a git header */
diff --git a/utf8.c b/utf8.c
index 8095a71..9efcdb9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -284,7 +284,6 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
  			text++;
  		}
  	}
-	return w;
  }

  int is_encoding_utf8(const char *name)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 5cb7171..1bad846 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -257,8 +257,6 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
  			return ec;
  		}
  	}
-
-	return -1;
  }


diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 2ade97b..d7974d1 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -232,8 +232,6 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
  		return i1 >= s1 && i2 >= s2;
  	} else
  		return s1 == s2 && !memcmp(l1, l2, s1);
-
-	return 0;
  }

  static unsigned long xdl_hash_record_with_whitespace(char const **data,
-- 
1.5.3.5.721.g039b

Re: [PATCH] Fix Solaris compiler warnings

[Alex Riesen]

Guido Ostkamp, Thu, Nov 15, 2007 23:19:11 +0100:
> Hello,
>
> the below patch fixes some compiler warnings returned by Solaris Workshop 
> Compilers.
>
>     CC builtin-apply.o
> "builtin-apply.c", line 686: warning: statement not reached
>     CC utf8.o
> "utf8.c", line 287: warning: statement not reached
>     CC xdiff/xdiffi.o
> "xdiff/xdiffi.c", line 261: warning: statement not reached

All these are wrong. That's a fantastically broken piece of compiler

>     CC xdiff/xutils.o
> "xdiff/xutils.c", line 236: warning: statement not reached

This one is right. Accidentally, as it seems

Re: [PATCH] Fix Solaris compiler warnings

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Guido Ostkamp, Thu, Nov 15, 2007 23:19:11 +0100:
>> Hello,
>>
>> the below patch fixes some compiler warnings returned by Solaris Workshop 
>> Compilers.
>>
>>     CC builtin-apply.o
>> "builtin-apply.c", line 686: warning: statement not reached
>>     CC utf8.o
>> "utf8.c", line 287: warning: statement not reached
>>     CC xdiff/xdiffi.o
>> "xdiff/xdiffi.c", line 261: warning: statement not reached
>
> All these are wrong. That's a fantastically broken piece of compiler

Eh?

I've looked at builtin-apply and utf8 cases but these returns
are after an endless loop whose exit paths always return
directly, so these return statements are in fact never reached.

Dumber compilers may not notice and if you remove these returns
they may start complaining, though. 

Re: [PATCH] Fix Solaris compiler warnings

[Alex Riesen]

Junio C Hamano, Fri, Nov 16, 2007 00:16:25 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > Guido Ostkamp, Thu, Nov 15, 2007 23:19:11 +0100:
> >> Hello,
> >>
> >> the below patch fixes some compiler warnings returned by Solaris Workshop 
> >> Compilers.
> >>
> >>     CC builtin-apply.o
> >> "builtin-apply.c", line 686: warning: statement not reached
> >>     CC utf8.o
> >> "utf8.c", line 287: warning: statement not reached
> >>     CC xdiff/xdiffi.o
> >> "xdiff/xdiffi.c", line 261: warning: statement not reached
> >
> > All these are wrong. That's a fantastically broken piece of compiler
> 
> Eh?
> 
> I've looked at builtin-apply and utf8 cases but these returns
> are after an endless loop whose exit paths always return
> directly, so these return statements are in fact never reached.
> 
> Dumber compilers may not notice and if you remove these returns
> they may start complaining, though. 

Hmm... Guido, I owe you an appology. Still, consider this patch
instead (it does not fix the return in xdiff/xdiffi.c though):


diff --git a/builtin-apply.c b/builtin-apply.c
index 8edcc08..6267396 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -668,13 +668,13 @@ static char *git_header_name(char *line, int llen)
 		default:
 			continue;
 		case '\n':
-			return NULL;
+			goto eol;
 		case '\t': case ' ':
 			second = name+len;
 			for (;;) {
 				char c = *second++;
 				if (c == '\n')
-					return NULL;
+					goto eol;
 				if (c == '/')
 					break;
 			}
@@ -683,6 +683,7 @@ static char *git_header_name(char *line, int llen)
 			}
 		}
 	}
+eol:
 	return NULL;
 }
 
diff --git a/utf8.c b/utf8.c
index 8095a71..50c46af 100644
--- a/utf8.c
+++ b/utf8.c
@@ -262,7 +262,7 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
 					print_spaces(indent);
 				fwrite(start, text - start, 1, stdout);
 				if (!c)
-					return w;
+					break;
 				else if (c == '\t')
 					w |= 0x07;
 				space = text;
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 2ade97b..533ff76 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -230,10 +230,9 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 			i2++;
 		}
 		return i1 >= s1 && i2 >= s2;
-	} else
-		return s1 == s2 && !memcmp(l1, l2, s1);
+	}
 
-	return 0;
+	return s1 == s2 && !memcmp(l1, l2, s1);
 }
 
 static unsigned long xdl_hash_record_with_whitespace(char const **data,

Re: [PATCH] Fix Solaris compiler warnings

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Fri, Nov 16, 2007 00:16:25 +0100:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>> 
>> > Guido Ostkamp, Thu, Nov 15, 2007 23:19:11 +0100:
>> ...
>> >>     CC builtin-apply.o
>> >> "builtin-apply.c", line 686: warning: statement not reached
>> >>     CC utf8.o
>> >> "utf8.c", line 287: warning: statement not reached
>> >>     CC xdiff/xdiffi.o
>> >> "xdiff/xdiffi.c", line 261: warning: statement not reached
>> >
>> > All these are wrong. That's a fantastically broken piece of compiler
>> 
>> I've looked at builtin-apply and utf8 cases but these returns
>> are after an endless loop whose exit paths always return
>> directly, so these return statements are in fact never reached.
>> ...
>
> Hmm... Guido, I owe you an appology. Still, consider this patch
> instead (it does not fix the return in xdiff/xdiffi.c though):

If you are referring to the "xdiff/xdiffi.c:line 261" one (which
I did not say if I looked at it or not), I think there is
nothing to fix there, either.  In front of itt is a big fat loop
controlled with:

	for (ec = 1;; ec++) {
		...
	}

and only exits from there are returns.  Two "break" appear but
they are breaking out of nested inner loops and would not escape
this outermost loop.

Re: [PATCH] Fix Solaris compiler warnings

[Guido Ostkamp]

On Fri, 16 Nov 2007, Alex Riesen wrote:
> Hmm... Guido, I owe you an appology.

accepted ;-)

> Still, consider this patch instead (it does not fix the return in 
> xdiff/xdiffi.c though):

If your intention is to have a final 'return' statement for stupid 
compilers, this should work (though I haven't verified it on a live system 
I think it looks reasonably well).

Are you going to include it?

What about the xdiff/xdiffi.c problem that should also be solved?

Regards

Guido

[PATCH] Rewrite some function exit paths to avoid "unreachable code" traps

[Alex Riesen]

Noticed by Guido Ostkamp for Sun's Workshop cc.

Originally-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Guido Ostkamp, Fri, Nov 16, 2007 23:52:01 +0100:
>
> What about the xdiff/xdiffi.c problem that should also be solved?
>

Here you go.

 builtin-apply.c |    5 +++--
 utf8.c          |    2 +-
 xdiff/xdiffi.c  |   14 +++++++-------
 xdiff/xutils.c  |    5 ++---
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 8edcc08..6267396 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -668,13 +668,13 @@ static char *git_header_name(char *line, int llen)
 		default:
 			continue;
 		case '\n':
-			return NULL;
+			goto eol;
 		case '\t': case ' ':
 			second = name+len;
 			for (;;) {
 				char c = *second++;
 				if (c == '\n')
-					return NULL;
+					goto eol;
 				if (c == '/')
 					break;
 			}
@@ -683,6 +683,7 @@ static char *git_header_name(char *line, int llen)
 			}
 		}
 	}
+eol:
 	return NULL;
 }
 
diff --git a/utf8.c b/utf8.c
index 8095a71..50c46af 100644
--- a/utf8.c
+++ b/utf8.c
@@ -262,7 +262,7 @@ int print_wrapped_text(const char *text, int indent, int indent2, int width)
 					print_spaces(indent);
 				fwrite(start, text - start, 1, stdout);
 				if (!c)
-					return w;
+					break;
 				else if (c == '\t')
 					w |= 0x07;
 				space = text;
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 5cb7171..365d768 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -110,7 +110,7 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
 				spl->i1 = i1;
 				spl->i2 = i2;
 				spl->min_lo = spl->min_hi = 1;
-				return ec;
+				goto end;
 			}
 		}
 
@@ -145,7 +145,7 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
 				spl->i1 = i1;
 				spl->i2 = i2;
 				spl->min_lo = spl->min_hi = 1;
-				return ec;
+				goto end;
 			}
 		}
 
@@ -184,7 +184,7 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
 			if (best > 0) {
 				spl->min_lo = 1;
 				spl->min_hi = 0;
-				return ec;
+				goto end;
 			}
 
 			for (best = 0, d = bmax; d >= bmin; d -= 2) {
@@ -208,7 +208,7 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
 			if (best > 0) {
 				spl->min_lo = 0;
 				spl->min_hi = 1;
-				return ec;
+				goto end;
 			}
 		}
 
@@ -254,11 +254,11 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
 				spl->min_lo = 0;
 				spl->min_hi = 1;
 			}
-			return ec;
+			goto end;
 		}
 	}
-
-	return -1;
+end:
+	return ec;
 }
 
 
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 2ade97b..533ff76 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -230,10 +230,9 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 			i2++;
 		}
 		return i1 >= s1 && i2 >= s2;
-	} else
-		return s1 == s2 && !memcmp(l1, l2, s1);
+	}
 
-	return 0;
+	return s1 == s2 && !memcmp(l1, l2, s1);
 }
 
 static unsigned long xdl_hash_record_with_whitespace(char const **data,
-- 
1.5.3.5.750.g9f37

Re: [PATCH] Rewrite some function exit paths to avoid "unreachable code" traps

[Robin Rosenberg]

lördag 17 november 2007 skrev Alex Riesen:
> Noticed by Guido Ostkamp for Sun's Workshop cc.
> 
> Originally-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
> Guido Ostkamp, Fri, Nov 16, 2007 23:52:01 +0100:
> >
> > What about the xdiff/xdiffi.c problem that should also be solved?
> >
> 

Please... This just looks bad. I'm sure we'll have fixup patches on the list
to fix those gotos. 

Do we support any such stupid compiler that requires a dummy goto?
If so we could just add a macro to compat-util.h

#if stupid_compiler
#define DUMMY_RETURN(x) return x;
#else
#define DUMMY_RETURN(x)
#endif

and then use it like this:

diff --git a/builtin-apply.c b/builtin-apply.c
index 8edcc08..91f8752 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -683,7 +683,6 @@ static char *git_header_name(char *line, int llen)
                        }
                }
        }
-       return NULL;
+      DUMMY_RETURN(NULL)
  }

My vote is for Guidos patch and fallback to the suggestion above if we support
really stupid compilers.

-- robin

Re: [PATCH] Rewrite some function exit paths to avoid "unreachable code" traps

[Alex Riesen]

Robin Rosenberg, Sat, Nov 17, 2007 11:39:32 +0100:
> lördag 17 november 2007 skrev Alex Riesen:
> > Noticed by Guido Ostkamp for Sun's Workshop cc.
> > 
> > Originally-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
> > Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> > ---
> > Guido Ostkamp, Fri, Nov 16, 2007 23:52:01 +0100:
> > >
> > > What about the xdiff/xdiffi.c problem that should also be solved?
> > >
> > 
> 
> Please... This just looks bad. I'm sure we'll have fixup patches on the list
> to fix those gotos. 
> 
> Do we support any such stupid compiler that requires a dummy goto?

It is more for the compilers we don't know about yet.

Userspace programming, especially with intent to be portable, often
means supporting *bugs* of the platform where it happens.

Re: [PATCH] Rewrite some function exit paths to avoid "unreachable code" traps

[Robin Rosenberg]

lördag 17 november 2007 skrev Alex Riesen:
> Robin Rosenberg, Sat, Nov 17, 2007 11:39:32 +0100:
> > lördag 17 november 2007 skrev Alex Riesen:
> > > Noticed by Guido Ostkamp for Sun's Workshop cc.
> > > 
> > > Originally-by: Guido Ostkamp <git@ostkamp.fastmail.fm>
> > > Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> > > ---
> > > Guido Ostkamp, Fri, Nov 16, 2007 23:52:01 +0100:
> > > >
> > > > What about the xdiff/xdiffi.c problem that should also be solved?
> > > >
> > > 
> > 
> > Please... This just looks bad. I'm sure we'll have fixup patches on the list
> > to fix those gotos. 
> > 
> > Do we support any such stupid compiler that requires a dummy goto?
> 
> It is more for the compilers we don't know about yet.
> 
> Userspace programming, especially with intent to be portable, often
> means supporting *bugs* of the platform where it happens.

*If* it happens. We do not workaround every hypothetical compiler bug or every hypotetical
buggy compiler. Compilers we don't know about does not "exist", expect for the perfectly
confirming ones, but they aren't buggy.

It seems the return in utf8.c was introduced by mistake and Junio has made his decision now.

-- robin