[PATCH 0/3] Fix a ton of compiler warnings

[PATCH 0/3] Fix a ton of compiler warnings

[Felipe Contreras]

Hi,

I'm getting tons and tons of warnings with gcc 4.9.0. Here are the patches to
fix them all.


Felipe Contreras (3):
  Revert "make error()'s constant return value more visible"
  Revert "silence some -Wuninitialized false positives"
  Silence a bunch of format-zero-length warnings

 builtin/commit.c  |  2 +-
 cache.h           |  3 ---
 config.c          |  1 -
 git-compat-util.h | 11 -----------
 parse-options.c   | 18 +++++++++---------
 parse-options.h   |  4 ----
 usage.c           |  1 -
 wt-status.c       | 22 +++++++++++-----------
 8 files changed, 21 insertions(+), 41 deletions(-)

-- 
1.9.2+fc1.20.g204a630

[PATCH 1/3] Revert "make error()'s constant return value more visible"

[Felipe Contreras]

In recent versions of gcc (4.9.0), we get hundreds of these:

  advice.c: In function ‘error_resolve_conflict’:
  advice.c:79:69: warning: right-hand operand of comma expression has no effect [-Wunused-value]
    error("'%s' is not possible because you have unmerged files.", me);
                                                                     ^
The original patch intended to help in situations like this:

  if (error(...))
    /* do stuff */

However, when there's no conditional statement this gets translated to:

  (error(..), 1);

And the right hand of the expression has no effect.

So it looks like gcc is smarter now, and in trying to fix a few warnings
we generated hundreds more.

This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.

Conflicts:
	git-compat-util.h
---
 git-compat-util.h | 11 -----------
 usage.c           |  1 -
 2 files changed, 12 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..b4242e4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -323,17 +323,6 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
 #include <openssl/x509v3.h>
 #endif /* NO_OPENSSL */
 
-/*
- * Let callers be aware of the constant return value; this can help
- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
- * because some compilers may not support variadic macros. Since we're only
- * trying to help gcc, anyway, it's OK; other compilers will fall back to
- * using the function as usual.
- */
-#if defined(__GNUC__) && ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
-#endif
-
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
diff --git a/usage.c b/usage.c
index ed14645..9d2961e 100644
--- a/usage.c
+++ b/usage.c
@@ -138,7 +138,6 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
-#undef error
 int error(const char *err, ...)
 {
 	va_list params;
-- 
1.9.2+fc1.20.g204a630

[PATCH 2/3] Revert "silence some -Wuninitialized false positives"

[Felipe Contreras]

In recent versions of gcc (4.9.0), we get a few of these:

  notes.c: In function ‘notes_display_config’:
  notes.c:970:28: warning: right-hand operand of comma expression has no effect [-Wunused-value]
      config_error_nonbool(k);
                            ^
Previous commit explains the reason.

This reverts commit a469a1019352b8efc4bd7003b0bd59eb60fc428c.

Conflicts:
	cache.h
	parse-options.h
---
 cache.h         |  3 ---
 config.c        |  1 -
 parse-options.c | 18 +++++++++---------
 parse-options.h |  4 ----
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index 107ac61..fae077a 100644
--- a/cache.h
+++ b/cache.h
@@ -1271,9 +1271,6 @@ extern int check_repository_format_version(const char *var, const char *value, v
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
-#if defined(__GNUC__) && ! defined(__clang__)
-#define config_error_nonbool(s) (config_error_nonbool(s), -1)
-#endif
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
diff --git a/config.c b/config.c
index a30cb5c..0083674 100644
--- a/config.c
+++ b/config.c
@@ -1870,7 +1870,6 @@ int git_config_rename_section(const char *old_name, const char *new_name)
  * Call this to report error for your variable that should not
  * get a boolean value (i.e. "[my] var" means "true").
  */
-#undef config_error_nonbool
 int config_error_nonbool(const char *var)
 {
 	return error("Missing value for '%s'", var);
diff --git a/parse-options.c b/parse-options.c
index b536896..2b4b8e5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -19,6 +19,15 @@ int optbug(const struct option *opt, const char *reason)
 	return error("BUG: switch '%c' %s", opt->short_name, reason);
 }
 
+int opterror(const struct option *opt, const char *reason, int flags)
+{
+	if (flags & OPT_SHORT)
+		return error("switch `%c' %s", opt->short_name, reason);
+	if (flags & OPT_UNSET)
+		return error("option `no-%s' %s", opt->long_name, reason);
+	return error("option `%s' %s", opt->long_name, reason);
+}
+
 static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
 		   int flags, const char **arg)
 {
@@ -632,12 +641,3 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 	return usage_with_options_internal(ctx, usagestr, opts, 0, err);
 }
 
-#undef opterror
-int opterror(const struct option *opt, const char *reason, int flags)
-{
-	if (flags & OPT_SHORT)
-		return error("switch `%c' %s", opt->short_name, reason);
-	if (flags & OPT_UNSET)
-		return error("option `no-%s' %s", opt->long_name, reason);
-	return error("option `%s' %s", opt->long_name, reason);
-}
diff --git a/parse-options.h b/parse-options.h
index 3189676..3b140d0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -176,10 +176,6 @@ extern NORETURN void usage_msg_opt(const char *msg,
 
 extern int optbug(const struct option *opt, const char *reason);
 extern int opterror(const struct option *opt, const char *reason, int flags);
-#if defined(__GNUC__) && ! defined(__clang__)
-#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
-#endif
-
 /*----- incremental advanced APIs -----*/
 
 enum {
-- 
1.9.2+fc1.20.g204a630

[PATCH 3/3] Silence a bunch of format-zero-length warnings

[Felipe Contreras]

This is in gcc 4.9.0:

  wt-status.c: In function ‘wt_status_print_unmerged_header’:
  wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]
    status_printf_ln(s, c, "");
    ^

We could pass -Wno-format-zero-length, but it seems compiler-specific
flags are frowned upon, so let's just avoid the warning altogether.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/commit.c |  2 +-
 wt-status.c      | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..13b84e4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -812,7 +812,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				committer_ident.buf);
 
 		if (ident_shown)
-			status_printf_ln(s, GIT_COLOR_NORMAL, "");
+			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
diff --git a/wt-status.c b/wt-status.c
index ec7344e..b8841e1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -188,7 +188,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 	} else {
 		status_printf_ln(s, c, _("  (use \"git add/rm <file>...\" as appropriate to mark resolution)"));
 	}
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_cached_header(struct wt_status *s)
@@ -204,7 +204,7 @@ static void wt_status_print_cached_header(struct wt_status *s)
 		status_printf_ln(s, c, _("  (use \"git reset %s <file>...\" to unstage)"), s->reference);
 	else
 		status_printf_ln(s, c, _("  (use \"git rm --cached <file>...\" to unstage)"));
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_dirty_header(struct wt_status *s,
@@ -223,7 +223,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 	status_printf_ln(s, c, _("  (use \"git checkout -- <file>...\" to discard changes in working directory)"));
 	if (has_dirty_submodules)
 		status_printf_ln(s, c, _("  (commit or discard the untracked or modified content in submodules)"));
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_other_header(struct wt_status *s,
@@ -235,12 +235,12 @@ static void wt_status_print_other_header(struct wt_status *s,
 	if (!s->hints)
 		return;
 	status_printf_ln(s, c, _("  (use \"git %s <file>...\" to include in what will be committed)"), how);
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_trailer(struct wt_status *s)
 {
-	status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
 
 #define quote_path quote_path_relative
@@ -826,7 +826,7 @@ static void wt_status_print_other(struct wt_status *s,
 	string_list_clear(&output, 0);
 	strbuf_release(&buf);
 conclude:
-	status_printf_ln(s, GIT_COLOR_NORMAL, "");
+	status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 }
 
 void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
@@ -913,7 +913,7 @@ static void wt_status_print_tracking(struct wt_status *s)
 		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
 				 comment_line_char);
 	else
-		fprintf_ln(s->fp, "");
+		fputs("", s->fp);
 }
 
 static int has_unmerged(struct wt_status *s)
@@ -1329,7 +1329,7 @@ void wt_status_print(struct wt_status *s)
 				on_what = _("Not currently on any branch.");
 			}
 		}
-		status_printf(s, color(WT_STATUS_HEADER, s), "");
+		status_printf(s, color(WT_STATUS_HEADER, s), "%s", "");
 		status_printf_more(s, branch_status_color, "%s", on_what);
 		status_printf_more(s, branch_color, "%s\n", branch_name);
 		if (!s->is_initial)
@@ -1342,9 +1342,9 @@ void wt_status_print(struct wt_status *s)
 	free(state.detached_from);
 
 	if (s->is_initial) {
-		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), _("Initial commit"));
-		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 	}
 
 	wt_status_print_updated(s);
@@ -1361,7 +1361,7 @@ void wt_status_print(struct wt_status *s)
 		if (s->show_ignored_files)
 			wt_status_print_other(s, &s->ignored, _("Ignored files"), "add -f");
 		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
-			status_printf_ln(s, GIT_COLOR_NORMAL, "");
+			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
 					   "may speed it up, but you have to be careful not to forget to add\n"
-- 
1.9.2+fc1.20.g204a630

Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:
> This is in gcc 4.9.0:
> 
>   wt-status.c: In function ‘wt_status_print_unmerged_header’:
>   wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]
>     status_printf_ln(s, c, "");
>     ^
> 
> We could pass -Wno-format-zero-length, but it seems compiler-specific
> flags are frowned upon, so let's just avoid the warning altogether.

I believe these warnings existed before GCC 4.9 as well, but I'm not
opposed to the change.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings

[Felipe Contreras]

brian m. carlson wrote:
> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:
> > This is in gcc 4.9.0:
> > 
> >   wt-status.c: In function ‘wt_status_print_unmerged_header’:
> >   wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]
> >     status_printf_ln(s, c, "");
> >     ^
> > 
> > We could pass -Wno-format-zero-length, but it seems compiler-specific
> > flags are frowned upon, so let's just avoid the warning altogether.
> 
> I believe these warnings existed before GCC 4.9 as well, but I'm not
> opposed to the change.

Yes, I'm aware of that. I didn't say they started to happen in 4.9, I
said they happen in 4.9.

-- 
Felipe Contreras

Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings

[Jeff King]

On Sun, May 04, 2014 at 07:01:22PM +0000, brian m. carlson wrote:

> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:
> > This is in gcc 4.9.0:
> > 
> >   wt-status.c: In function ‘wt_status_print_unmerged_header’:
> >   wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]
> >     status_printf_ln(s, c, "");
> >     ^
> > 
> > We could pass -Wno-format-zero-length, but it seems compiler-specific
> > flags are frowned upon, so let's just avoid the warning altogether.
> 
> I believe these warnings existed before GCC 4.9 as well, but I'm not
> opposed to the change.

Yeah, this started last summer when we added __attribute__((format)) to
the status_printf_ln calls, and I posted essentially the same patch.  We
kind of waffled between "eh, just set -Wno-format-zero-length" and doing
something, and ended up at the former. I'd be fine with doing it this
way; we're not likely to add a lot of new callsites that would make it a
hassle to keep up with.

-Peff

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Felipe Contreras]

Jeff King wrote:
> On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:
> 
> > So it looks like gcc is smarter now, and in trying to fix a few warnings
> > we generated hundreds more.
> > 
> > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.
> 
> And now we've gone the other way, and re-enabled the initial warnings.
> Can we come up with a solution that helps both cases?

What initial warnings? As I explained already I don't get any warnings
with this patch series in gcc 4.9.0.

-- 
Felipe Contreras

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Jeff King]

On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:

> So it looks like gcc is smarter now, and in trying to fix a few warnings
> we generated hundreds more.
> 
> This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.

And now we've gone the other way, and re-enabled the initial warnings.
Can we come up with a solution that helps both cases?

I could not find a way to annotate a value as "maybe unused", but we can
hide it inside a function, like:

-- >8 --
Subject: inline error() function

The error() function does two things: it prints an error,
and it returns "-1" as a convenience to allow:

  return error("foo");

Commit e208f9c converted this to a macro to make the
constant return value more visible to the compiler. However,
recent versions of gcc complain when error is used in a void
context, as the constant "-1" ends up unused.

Instead, let's convert error() to a static inline, which
should accomplish the same thing without the extra warnings
(because gcc will not warn about unused return values unless
warn_unused_result is specified for the particular
function).

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 20 ++++++++++++--------
 usage.c           |  8 +-------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..3aef0d3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -310,7 +310,6 @@ extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
 #ifndef NO_OPENSSL
@@ -325,14 +324,19 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
 
 /*
  * Let callers be aware of the constant return value; this can help
- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
- * because some compilers may not support variadic macros. Since we're only
- * trying to help gcc, anyway, it's OK; other compilers will fall back to
- * using the function as usual.
+ * gcc with -Wuninitialized analysis.
  */
-#if defined(__GNUC__) && ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
-#endif
+__attribute__((format (printf, 1, 0)))
+extern void error_impl(const char *err, va_list params);
+__attribute__((format (printf, 1, 2)))
+static inline int error(const char *err, ...)
+{
+    va_list params;
+    va_start(params, err);
+    error_impl(err, params);
+    va_end(params);
+    return -1;
+}
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list params));
diff --git a/usage.c b/usage.c
index ed14645..5456e4b 100644
--- a/usage.c
+++ b/usage.c
@@ -138,15 +138,9 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
-#undef error
-int error(const char *err, ...)
+void error_impl(const char *err, va_list params)
 {
-	va_list params;
-
-	va_start(params, err);
 	error_routine(err, params);
-	va_end(params);
-	return -1;
 }
 
 void warning(const char *warn, ...)
-- 
2.0.0.rc1.436.g03cb729

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Jeff King]

On Mon, May 05, 2014 at 12:45:30AM -0500, Felipe Contreras wrote:

> Jeff King wrote:
> > On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:
> > 
> > > So it looks like gcc is smarter now, and in trying to fix a few warnings
> > > we generated hundreds more.
> > > 
> > > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.
> > 
> > And now we've gone the other way, and re-enabled the initial warnings.
> > Can we come up with a solution that helps both cases?
> 
> What initial warnings? As I explained already I don't get any warnings
> with this patch series in gcc 4.9.0.

The "few warnings" in your statement quoted above.

You could try reading the commit message of the commit you are
reverting, which explains it, but the short answer is: try compiling
with -O3.

-Peff

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Felipe Contreras]

Jeff King wrote:
> On Mon, May 05, 2014 at 12:45:30AM -0500, Felipe Contreras wrote:
> 
> > Jeff King wrote:
> > > On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:
> > > 
> > > > So it looks like gcc is smarter now, and in trying to fix a few warnings
> > > > we generated hundreds more.
> > > > 
> > > > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.
> > > 
> > > And now we've gone the other way, and re-enabled the initial warnings.
> > > Can we come up with a solution that helps both cases?
> > 
> > What initial warnings? As I explained already I don't get any warnings
> > with this patch series in gcc 4.9.0.
> 
> The "few warnings" in your statement quoted above.
> 
> You could try reading the commit message of the commit you are
> reverting, which explains it, but the short answer is: try compiling
> with -O3.

Sigh. And I'm the one with the abrasive style of communication.

-- 
Felipe Contreras

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Jeff King]

On Mon, May 05, 2014 at 01:14:43AM -0500, Felipe Contreras wrote:

> Jeff King wrote:
> > On Mon, May 05, 2014 at 12:45:30AM -0500, Felipe Contreras wrote:
> > 
> > > Jeff King wrote:
> > > > On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:
> > > > 
> > > > > So it looks like gcc is smarter now, and in trying to fix a few warnings
> > > > > we generated hundreds more.
> > > > > 
> > > > > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.
> > > > 
> > > > And now we've gone the other way, and re-enabled the initial warnings.
> > > > Can we come up with a solution that helps both cases?
> > > 
> > > What initial warnings? As I explained already I don't get any warnings
> > > with this patch series in gcc 4.9.0.
> > 
> > The "few warnings" in your statement quoted above.
> > 
> > You could try reading the commit message of the commit you are
> > reverting, which explains it, but the short answer is: try compiling
> > with -O3.
> 
> Sigh. And I'm the one with the abrasive style of communication.

I apologize if that seemed abrasive. I am slightly annoyed that you
seemed to be reverting my commit without understanding (or dealing with)
the problem that the original fixed.

But I was _also_ trying to point you in the right direction by directing
you to -O3. Do you see the problem now?  And did you look at the
follow-up patch I sent?

-Peff

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Felipe Contreras]

Jeff King wrote:
> On Mon, May 05, 2014 at 01:14:43AM -0500, Felipe Contreras wrote:
> > Jeff King wrote:

> > > You could try reading the commit message of the commit you are
> > > reverting, which explains it, but the short answer is: try compiling
> > > with -O3.
> > 
> > Sigh. And I'm the one with the abrasive style of communication.
> 
> I apologize if that seemed abrasive. I am slightly annoyed that you
> seemed to be reverting my commit without understanding (or dealing with)
> the problem that the original fixed.

The original problem happens only with -O3, I tried only with -O2 and
didn't see any problems.

If we have a) code that fixes a couple warnings with -O3 but introduces
hundreds with -O2, vs. b) code that has only a comple warnings with -O3,
I'd go for b) any day.

> But I was _also_ trying to point you in the right direction by directing
> you to -O3. Do you see the problem now?  And did you look at the
> follow-up patch I sent?

Yes, I see the problem now with -O3. And yes, I looked at the patch you
sent. I haven't tested it but I bet it would sove both problems.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/3] Revert "make error()'s constant return value more visible"

[Jeff King]

On Mon, May 05, 2014 at 02:30:12AM -0500, Felipe Contreras wrote:

> If we have a) code that fixes a couple warnings with -O3 but introduces
> hundreds with -O2, vs. b) code that has only a comple warnings with -O3,
> I'd go for b) any day.

I agree. But my point was to ask whether we can fix both.

> Yes, I see the problem now with -O3. And yes, I looked at the patch you
> sent. I haven't tested it but I bet it would sove both problems.

Unfortunately, it does not work in all cases (and I obviously did
something wrong when testing it last night). I should have taken my own
advice and re-read the commit message for e208f9c more carefully, which
says:

    If we can make the compiler aware that error() will always
    return -1, it can do a better job of analysis. The simplest
    method would be to inline the error() function. However,
    this doesn't work, because gcc will not inline a variadc
    function. We can work around this by defining a macro.[...]

I cannot think of any other way to make the compiler aware of the
constant value, but perhaps somebody else is more clever than I am.
Another alternative is to write out "return error(...)" as "error(...);
return -1". In some ways that is more readable, though it is more
verbose (and would cause quite a bit of code churn).

So applying your patches may be the least-bad solution.

-Peff

[PATCH 1/2] inline constant return from error() function

[Jeff King]

Commit e208f9c introduced a macro to turn error() calls
into:

  (error(), -1)

to make the constant return value more visible to the
calling code (and thus let the compiler make better
decisions about the code).

This works well for code like:

  return error(...);

but the "-1" is superfluous in code that just calls error()
without caring about the return value. In older versions of
gcc, that was fine, but gcc 4.9 complains with -Wunused-value.

We can work around this by encapsulating the constant return
value in a static inline function, as gcc specifically
avoids complaining about unused function returns unless the
function has been specifically marked with the
warn_unused_result attribute.

We also use the same trick for config_error_nonbool and
opterror, which learned the same error technique in a469a10.

Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:

> I cannot think of any other way to make the compiler aware of the
> constant value, but perhaps somebody else is more clever than I am.

This came to me in a dream, and seems to work.

 cache.h           | 2 +-
 git-compat-util.h | 6 +++++-
 parse-options.h   | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 107ac61..e2f12b0 100644
--- a/cache.h
+++ b/cache.h
@@ -1272,7 +1272,7 @@ extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
 #if defined(__GNUC__) && ! defined(__clang__)
-#define config_error_nonbool(s) (config_error_nonbool(s), -1)
+#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
 #endif
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..b4c437e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
  * using the function as usual.
  */
 #if defined(__GNUC__) && ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
+static inline int const_error(void)
+{
+	return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
 #endif
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
diff --git a/parse-options.h b/parse-options.h
index 3189676..2f9be96 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,7 +177,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
 extern int optbug(const struct option *opt, const char *reason);
 extern int opterror(const struct option *opt, const char *reason, int flags);
 #if defined(__GNUC__) && ! defined(__clang__)
-#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
+#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
 #endif
 
 /*----- incremental advanced APIs -----*/
-- 
2.0.0.rc1.436.g03cb729

[PATCH 2/2] let clang use the constant-return error() macro

[Jeff King]

Commit e208f9c converted error() into a macro to make its
constant return value more apparent to calling code.  Commit
5ded807 prevents us using this macro with clang, since
clang's -Wunused-value is smart enough to realize that the
constant "-1" is useless in some contexts.

However, since the last commit puts the constant behind an
inline function call, this is enough to prevent the
-Wunused-value warning on both modern gcc and clang. So we
can now re-enable the macro when compiling with clang.

Tested with clang 3.3, 3.4, and 3.5.

Signed-off-by: Jeff King <peff@peff.net>
---
I still get warnings when compiling with clang -O3, due to
-Warray-bounds. It looks like a bug, though. Clang complains that:

  strcmp(argv[1], "git")

oversteps array bounds when the strcmp is expanded into a mess of
builtin magic. So I don't think we are doing anything wrong here.

 cache.h           | 2 +-
 git-compat-util.h | 2 +-
 parse-options.h   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index e2f12b0..35a3e6b 100644
--- a/cache.h
+++ b/cache.h
@@ -1271,7 +1271,7 @@ extern int check_repository_format_version(const char *var, const char *value, v
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
-#if defined(__GNUC__) && ! defined(__clang__)
+#if defined(__GNUC__)
 #define config_error_nonbool(s) (config_error_nonbool(s), const_error())
 #endif
 extern const char *get_log_output_encoding(void);
diff --git a/git-compat-util.h b/git-compat-util.h
index b4c437e..70dc028 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -330,7 +330,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
  * trying to help gcc, anyway, it's OK; other compilers will fall back to
  * using the function as usual.
  */
-#if defined(__GNUC__) && ! defined(__clang__)
+#if defined(__GNUC__)
 static inline int const_error(void)
 {
 	return -1;
diff --git a/parse-options.h b/parse-options.h
index 2f9be96..7940bc7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -176,7 +176,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
 
 extern int optbug(const struct option *opt, const char *reason);
 extern int opterror(const struct option *opt, const char *reason, int flags);
-#if defined(__GNUC__) && ! defined(__clang__)
+#if defined(__GNUC__)
 #define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
 #endif
 
-- 
2.0.0.rc1.436.g03cb729

Re: [PATCH 1/2] inline constant return from error() function

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Commit e208f9c introduced a macro to turn error() calls
> into:
>
>   (error(), -1)
>
> to make the constant return value more visible to the
> calling code (and thus let the compiler make better
> decisions about the code).
>
> This works well for code like:
>
>   return error(...);
>
> but the "-1" is superfluous in code that just calls error()
> without caring about the return value. In older versions of
> gcc, that was fine, but gcc 4.9 complains with -Wunused-value.
>
> We can work around this by encapsulating the constant return
> value in a static inline function, as gcc specifically
> avoids complaining about unused function returns unless the
> function has been specifically marked with the
> warn_unused_result attribute.

That's kind of W*A*T magic, and I generally try to avoid magic, as
long as it solves your "can we make both -O2 with new compilers and
-O3 happy?" I wouldn't complain ;-)

> We also use the same trick for config_error_nonbool and
> opterror, which learned the same error technique in a469a10.
>
> Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:
>
>> I cannot think of any other way to make the compiler aware of the
>> constant value, but perhaps somebody else is more clever than I am.
>
> This came to me in a dream, and seems to work.



>
>  cache.h           | 2 +-
>  git-compat-util.h | 6 +++++-
>  parse-options.h   | 2 +-
>  3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 107ac61..e2f12b0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1272,7 +1272,7 @@ extern int git_env_bool(const char *, int);
>  extern int git_config_system(void);
>  extern int config_error_nonbool(const char *);
>  #if defined(__GNUC__) && ! defined(__clang__)
> -#define config_error_nonbool(s) (config_error_nonbool(s), -1)
> +#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
>  #endif
>  extern const char *get_log_output_encoding(void);
>  extern const char *get_commit_output_encoding(void);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f6d3a46..b4c437e 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
>   * using the function as usual.
>   */
>  #if defined(__GNUC__) && ! defined(__clang__)
> -#define error(...) (error(__VA_ARGS__), -1)
> +static inline int const_error(void)
> +{
> +	return -1;
> +}
> +#define error(...) (error(__VA_ARGS__), const_error())
>  #endif
>  
>  extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
> diff --git a/parse-options.h b/parse-options.h
> index 3189676..2f9be96 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -177,7 +177,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
>  extern int optbug(const struct option *opt, const char *reason);
>  extern int opterror(const struct option *opt, const char *reason, int flags);
>  #if defined(__GNUC__) && ! defined(__clang__)
> -#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
> +#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
>  #endif
>  
>  /*----- incremental advanced APIs -----*/

Re: [PATCH 1/2] inline constant return from error() function

[Jeff King]

On Tue, May 06, 2014 at 03:29:37PM -0700, Junio C Hamano wrote:

> > We can work around this by encapsulating the constant return
> > value in a static inline function, as gcc specifically
> > avoids complaining about unused function returns unless the
> > function has been specifically marked with the
> > warn_unused_result attribute.
> 
> That's kind of W*A*T magic, and I generally try to avoid magic, as
> long as it solves your "can we make both -O2 with new compilers and
> -O3 happy?" I wouldn't complain ;-)

I agree it's rather magical, but I think it's something we can count on.
Certainly turning on warn_unused_result for every function would be a
catastrophe for most code bases, and I don't expect gcc to do it. It's
possible it would eventually grow smart to say "eh, I inlined this and
realized that you don't use the return value", but I think that would be
similarly a bad idea.

And it does work with -O2 and -O3 with both gcc-4.9 and clang in my
tests.

-Peff

Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, May 04, 2014 at 07:01:22PM +0000, brian m. carlson wrote:
>
>> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:
>> > This is in gcc 4.9.0:
>> > 
>> >   wt-status.c: In function ‘wt_status_print_unmerged_header’:
>> >   wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]
>> >     status_printf_ln(s, c, "");
>> >     ^
>> > 
>> > We could pass -Wno-format-zero-length, but it seems compiler-specific
>> > flags are frowned upon, so let's just avoid the warning altogether.
>> 
>> I believe these warnings existed before GCC 4.9 as well, but I'm not
>> opposed to the change.
>
> Yeah, this started last summer when we added __attribute__((format)) to
> the status_printf_ln calls, and I posted essentially the same patch.  We
> kind of waffled between "eh, just set -Wno-format-zero-length" and doing
> something, and ended up at the former. I'd be fine with doing it this
> way; we're not likely to add a lot of new callsites that would make it a
> hassle to keep up with.

OK, so I'll take it as your Ack ;-)

Re: Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings

[Heiko Voigt]

On Wed, May 07, 2014 at 11:19:09AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Sun, May 04, 2014 at 07:01:22PM +0000, brian m. carlson wrote:
> >
> >> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:
> >> > This is in gcc 4.9.0:
> >> > 
> >> >   wt-status.c: In function ‘wt_status_print_unmerged_header’:
> >> >   wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]
> >> >     status_printf_ln(s, c, "");
> >> >     ^
> >> > 
> >> > We could pass -Wno-format-zero-length, but it seems compiler-specific
> >> > flags are frowned upon, so let's just avoid the warning altogether.
> >> 
> >> I believe these warnings existed before GCC 4.9 as well, but I'm not
> >> opposed to the change.
> >
> > Yeah, this started last summer when we added __attribute__((format)) to
> > the status_printf_ln calls, and I posted essentially the same patch.  We
> > kind of waffled between "eh, just set -Wno-format-zero-length" and doing
> > something, and ended up at the former. I'd be fine with doing it this
> > way; we're not likely to add a lot of new callsites that would make it a
> > hassle to keep up with.
> 
> OK, so I'll take it as your Ack ;-)

What happened to this patch? These warnings are still annoying me on my
Ubuntu 14.04.

Cheers Heiko

Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> On Wed, May 07, 2014 at 11:19:09AM -0700, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> ...
>> > Yeah, this started last summer when we added __attribute__((format)) to
>> > the status_printf_ln calls, and I posted essentially the same patch.  We
>> > kind of waffled between "eh, just set -Wno-format-zero-length" and doing
>> > something, and ended up at the former. I'd be fine with doing it this
>> > way; we're not likely to add a lot of new callsites that would make it a
>> > hassle to keep up with.
>> 
>> OK, so I'll take it as your Ack ;-)
>
> What happened to this patch? These warnings are still annoying me on my
> Ubuntu 14.04.

As Peff summarized, an earlier patch was dropped with "It is easy
for builders with such compilers to squelch the warning."  We are
reversing the course, with an updated log message like this ;-)

   The user have long been told to pass -Wno-format-zero-length, but a
   patch that avoids warning altogether is not too noisy, so let's do
   so.

Re: [PATCH 1/2] inline constant return from error() function

[Felipe Contreras]

Junio C Hamano wrote:
> That's kind of W*A*T magic, and I generally try to avoid magic, as
> long as it solves your "can we make both -O2 with new compilers and
> -O3 happy?" I wouldn't complain ;-)

In case anybody is looking for a non-hacky way of doing this that
doesn't depend on the gcc version of the week, this is how I silenced
the warnings in git-fc:

https://github.com/felipec/git/commit/e14ca39ba0092f0305d5fb347e20b829f736b29b

-- 
Felipe Contreras

Re: [PATCH 1/2] inline constant return from error() function

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 06, 2014 at 03:29:37PM -0700, Junio C Hamano wrote:
>
>> That's kind of W*A*T magic, and I generally try to avoid magic, as
>> long as it solves your "can we make both -O2 with new compilers and
>> -O3 happy?" I wouldn't complain ;-)
>
> I agree it's rather magical, but I think it's something we can count on.

Certainly. Sorry that I missed "but" before "as long as", which made
me sound as if I were unhappy.  At least, I didn't call it an ugly
"hack" ;-)

The alternative you mentioned up-thread "... to write out "return
error(...)"  as "error(...); return -1". In some ways that is more
readable, though it is more verbose..." has one more downside you
did not mention, and the approach to encapsulate it inside error()
will not have it: new call-sites to error() do not have to worry
about the issue with this approach.

Until it breaks, that is.  But that goes without saying with the
"it's something we can count on" pre-condition in place ;-).

Re: [PATCH 1/2] inline constant return from error() function

[Jeff King]

On Sun, May 11, 2014 at 10:22:03AM -0700, Junio C Hamano wrote:

> The alternative you mentioned up-thread "... to write out "return
> error(...)"  as "error(...); return -1". In some ways that is more
> readable, though it is more verbose..." has one more downside you
> did not mention, and the approach to encapsulate it inside error()
> will not have it: new call-sites to error() do not have to worry
> about the issue with this approach.
> 
> Until it breaks, that is.  But that goes without saying with the
> "it's something we can count on" pre-condition in place ;-).

Yeah, I agree with this thinking. I'd rather not do something that
impacts each callsite until we have exhausted other options that hide
the complexity in the definition.

-Peff

Re: [PATCH 1/2] inline constant return from error() function

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:

>> I cannot think of any other way to make the compiler aware of the
>> constant value, but perhaps somebody else is more clever than I am.
>
> This came to me in a dream, and seems to work.

Clever. :)  Thanks for thinking it up.

[...]
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
>   * using the function as usual.
>   */
>  #if defined(__GNUC__) && ! defined(__clang__)
> -#define error(...) (error(__VA_ARGS__), -1)
> +static inline int const_error(void)
> +{
> +	return -1;
> +}
> +#define error(...) (error(__VA_ARGS__), const_error())

I wish we could just make error() an inline function that calls some
print_error() helper, but I don't believe all compilers used to build
git are smart enough to inline uses of varargs.  So this looks like
the right thing to do.

I kind of wish we weren't in so much of an arms race with static
analyzers.  Is there some standard way to submit our code as "an idiom
that should continue not to produce warnings" to be included in a
testsuite and prevent problems in the future?

Thanks,
Jonathan

Re: [PATCH 1/2] inline constant return from error() function

[Jeff King]

On Mon, May 12, 2014 at 11:44:26AM -0700, Jonathan Nieder wrote:

> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
> >   * using the function as usual.
> >   */
> >  #if defined(__GNUC__) && ! defined(__clang__)
> > -#define error(...) (error(__VA_ARGS__), -1)
> > +static inline int const_error(void)
> > +{
> > +	return -1;
> > +}
> > +#define error(...) (error(__VA_ARGS__), const_error())
> 
> I wish we could just make error() an inline function that calls some
> print_error() helper, but I don't believe all compilers used to build
> git are smart enough to inline uses of varargs.  So this looks like
> the right thing to do.

Not just "not all compilers", but "not even gcc". If you declare it
always_inline, gcc will even complain "you can't do that, it has
varargs".

For my curiosity, is there any compiler which _does_ inline varargs
calls? Clang would be the obvious guess.

> I kind of wish we weren't in so much of an arms race with static
> analyzers.  Is there some standard way to submit our code as "an idiom
> that should continue not to produce warnings" to be included in a
> testsuite and prevent problems in the future?

I agree the arms race is frustrating, but I think the compiler is being
completely reasonable in this case. It has flagged code where it knows
a variable may be uninitialized depending on the return value from
error(). The only reason I as a human know it's a false positive is that
I know error() will always return -1. So it's not an idiom here, so much
as letting the compiler know everything we know.

It would just be nice if there was an easier way to do it.

I do think giving the compiler that information is nicer than an
attribute saying "trust me, it's initialized". The constant return not
only silences the warnings, but it may actually let the compiler produce
better code (both in these cases, but in the hundreds of other spots
that call error()).

-Peff