threads / patch / 10892

patchFix Solaris compiler warnings

Subject: [PATCH] Fix Solaris compiler warnings

## tl;dr

10 messages between Nov 15, 2007 and Nov 17, 2007. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Guido Ostkamp· Nov 15, 2007, 22:19 UTC · lore

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(-)

Show changes to 4 files +0 −6

builtin-apply.c, utf8.c, xdiff/xdiffi.c, xdiff/xutils.c

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

Alex Riesen· Nov 15, 2007, 23:00 UTC · re: Guido Ostkamp · lore

Re: [PATCH] Fix Solaris compiler warnings

Guido Ostkamp, Thu, Nov 15, 2007 23:19:11 +0100:

Show 11 quoted lines

> 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

Junio C Hamano· Nov 15, 2007, 23:16 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix Solaris compiler warnings

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

Show 14 quoted lines

> 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.

Alex Riesen· Nov 16, 2007, 07:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix Solaris compiler warnings

Junio C Hamano, Fri, Nov 16, 2007 00:16:25 +0100:

Show 25 quoted lines

> 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):

Show changes to 3 files +6 −6

builtin-apply.c, utf8.c, xdiff/xutils.c

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,

Junio C Hamano· Nov 16, 2007, 09:14 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix Solaris compiler warnings

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

Show 21 quoted lines

> 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.

Guido Ostkamp· Nov 16, 2007, 22:52 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix Solaris compiler warnings

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

Alex Riesen· Nov 17, 2007, 09:46 UTC · re: Guido Ostkamp · lore

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

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(-)

Show changes to 4 files +13 −13

builtin-apply.c, utf8.c, xdiff/xdiffi.c, xdiff/xutils.c

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

Robin Rosenberg· Nov 17, 2007, 10:39 UTC · re: Alex Riesen · lore

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

lördag 17 november 2007 skrev Alex Riesen:

Show 10 quoted lines

> 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:

Show changes to builtin-apply.c +1 −2

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

Alex Riesen· Nov 17, 2007, 12:23 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg, Sat, Nov 17, 2007 11:39:32 +0100:

Show 16 quoted lines

> 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.

Robin Rosenberg· Nov 17, 2007, 14:31 UTC · re: Alex Riesen · lore

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

lördag 17 november 2007 skrev Alex Riesen:

Show 22 quoted lines

> 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