{"thread":{"id":"36579","subject":"[PATCH 0/3] Fix a ton of compiler warnings","startedAt":"2014-05-04T06:12:52Z","lastAt":"2014-05-12T18:56:45Z","messageCount":26,"participants":["Felipe Contreras","brian m. carlson","Jeff King","Junio C Hamano","Heiko Voigt","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"240664","messageId":"1399183975-2346-1-git-send-email-felipe.contreras@gmail.com","threadId":"36579","inReplyTo":null,"subject":"[PATCH 0/3] Fix a ton of compiler warnings","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-04T06:12:52Z","receivedAt":"2014-05-04T06:12:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nI'm getting tons and tons of warnings with gcc 4.9.0. Here are the patches to\nfix them all.\n\n\nFelipe Contreras (3):\n Revert \"make error()'s constant return value more visible\"\n Revert \"silence some -Wuninitialized false positives\"\n Silence a bunch of format-zero-length warnings\n\n builtin/commit.c | 2 +-\n cache.h | 3 ---\n config.c | 1 -\n git-compat-util.h | 11 -----------\n parse-options.c | 18 +++++++++---------\n parse-options.h | 4 ----\n usage.c | 1 -\n wt-status.c | 22 +++++++++++-----------\n 8 files changed, 21 insertions(+), 41 deletions(-)\n\n-- \n1.9.2+fc1.20.g204a630\n"},{"id":"240667","messageId":"1399183975-2346-2-git-send-email-felipe.contreras@gmail.com","threadId":"36579","inReplyTo":"1399183975-2346-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-04T06:12:53Z","receivedAt":"2014-05-04T06:12:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"In recent versions of gcc (4.9.0), we get hundreds of these:\n\n advice.c: In function ‘error_resolve_conflict’:\n advice.c:79:69: warning: right-hand operand of comma expression has no effect [-Wunused-value]\n error(\"'%s' is not possible because you have unmerged files.\", me);\n ^\nThe original patch intended to help in situations like this:\n\n if (error(...))\n /* do stuff */\n\nHowever, when there's no conditional statement this gets translated to:\n\n (error(..), 1);\n\nAnd the right hand of the expression has no effect.\n\nSo it looks like gcc is smarter now, and in trying to fix a few warnings\nwe generated hundreds more.\n\nThis reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.\n\nConflicts:\n\tgit-compat-util.h\n---\n git-compat-util.h | 11 -----------\n usage.c | 1 -\n 2 files changed, 12 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f6d3a46..b4242e4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -323,17 +323,6 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n #include \n #endif /* NO_OPENSSL */\n \n-/*\n- * Let callers be aware of the constant return value; this can help\n- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,\n- * because some compilers may not support variadic macros. Since we're only\n- * trying to help gcc, anyway, it's OK; other compilers will fall back to\n- * using the function as usual.\n- */\n-#if defined(__GNUC__) && ! defined(__clang__)\n-#define error(...) (error(__VA_ARGS__), -1)\n-#endif\n-\n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\n extern void set_die_is_recursing_routine(int (*routine)(void));\ndiff --git a/usage.c b/usage.c\nindex ed14645..9d2961e 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -138,7 +138,6 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n-#undef error\n int error(const char *err, ...)\n {\n \tva_list params;\n-- \n1.9.2+fc1.20.g204a630\n"},{"id":"240665","messageId":"1399183975-2346-3-git-send-email-felipe.contreras@gmail.com","threadId":"36579","inReplyTo":"1399183975-2346-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/3] Revert \"silence some -Wuninitialized false positives\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-04T06:12:54Z","receivedAt":"2014-05-04T06:12:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"In recent versions of gcc (4.9.0), we get a few of these:\n\n notes.c: In function ‘notes_display_config’:\n notes.c:970:28: warning: right-hand operand of comma expression has no effect [-Wunused-value]\n config_error_nonbool(k);\n ^\nPrevious commit explains the reason.\n\nThis reverts commit a469a1019352b8efc4bd7003b0bd59eb60fc428c.\n\nConflicts:\n\tcache.h\n\tparse-options.h\n---\n cache.h | 3 ---\n config.c | 1 -\n parse-options.c | 18 +++++++++---------\n parse-options.h | 4 ----\n 4 files changed, 9 insertions(+), 17 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..fae077a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1271,9 +1271,6 @@ extern int check_repository_format_version(const char *var, const char *value, v\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n-#if defined(__GNUC__) && ! defined(__clang__)\n-#define config_error_nonbool(s) (config_error_nonbool(s), -1)\n-#endif\n extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \ndiff --git a/config.c b/config.c\nindex a30cb5c..0083674 100644\n--- a/config.c\n+++ b/config.c\n@@ -1870,7 +1870,6 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n * Call this to report error for your variable that should not\n * get a boolean value (i.e. \"[my] var\" means \"true\").\n */\n-#undef config_error_nonbool\n int config_error_nonbool(const char *var)\n {\n \treturn error(\"Missing value for '%s'\", var);\ndiff --git a/parse-options.c b/parse-options.c\nindex b536896..2b4b8e5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -19,6 +19,15 @@ int optbug(const struct option *opt, const char *reason)\n \treturn error(\"BUG: switch '%c' %s\", opt->short_name, reason);\n }\n \n+int opterror(const struct option *opt, const char *reason, int flags)\n+{\n+\tif (flags & OPT_SHORT)\n+\t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\n+\tif (flags & OPT_UNSET)\n+\t\treturn error(\"option `no-%s' %s\", opt->long_name, reason);\n+\treturn error(\"option `%s' %s\", opt->long_name, reason);\n+}\n+\n static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,\n \t\t int flags, const char **arg)\n {\n@@ -632,12 +641,3 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \treturn usage_with_options_internal(ctx, usagestr, opts, 0, err);\n }\n \n-#undef opterror\n-int opterror(const struct option *opt, const char *reason, int flags)\n-{\n-\tif (flags & OPT_SHORT)\n-\t\treturn error(\"switch `%c' %s\", opt->short_name, reason);\n-\tif (flags & OPT_UNSET)\n-\t\treturn error(\"option `no-%s' %s\", opt->long_name, reason);\n-\treturn error(\"option `%s' %s\", opt->long_name, reason);\n-}\ndiff --git a/parse-options.h b/parse-options.h\nindex 3189676..3b140d0 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -176,10 +176,6 @@ extern NORETURN void usage_msg_opt(const char *msg,\n \n extern int optbug(const struct option *opt, const char *reason);\n extern int opterror(const struct option *opt, const char *reason, int flags);\n-#if defined(__GNUC__) && ! defined(__clang__)\n-#define opterror(o,r,f) (opterror((o),(r),(f)), -1)\n-#endif\n-\n /*----- incremental advanced APIs -----*/\n \n enum {\n-- \n1.9.2+fc1.20.g204a630\n"},{"id":"240666","messageId":"1399183975-2346-4-git-send-email-felipe.contreras@gmail.com","threadId":"36579","inReplyTo":"1399183975-2346-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-04T06:12:55Z","receivedAt":"2014-05-04T06:12:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This is in gcc 4.9.0:\n\n wt-status.c: In function ‘wt_status_print_unmerged_header’:\n wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n status_printf_ln(s, c, \"\");\n ^\n\nWe could pass -Wno-format-zero-length, but it seems compiler-specific\nflags are frowned upon, so let's just avoid the warning altogether.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/commit.c | 2 +-\n wt-status.c | 22 +++++++++++-----------\n 2 files changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9cfef6c..13b84e4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -812,7 +812,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tcommitter_ident.buf);\n \n \t\tif (ident_shown)\n-\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n \n \t\tsaved_color_setting = s->use_color;\n \t\ts->use_color = 0;\ndiff --git a/wt-status.c b/wt-status.c\nindex ec7344e..b8841e1 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -188,7 +188,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s)\n \t} else {\n \t\tstatus_printf_ln(s, c, _(\" (use \\\"git add/rm ...\\\" as appropriate to mark resolution)\"));\n \t}\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_cached_header(struct wt_status *s)\n@@ -204,7 +204,7 @@ static void wt_status_print_cached_header(struct wt_status *s)\n \t\tstatus_printf_ln(s, c, _(\" (use \\\"git reset %s ...\\\" to unstage)\"), s->reference);\n \telse\n \t\tstatus_printf_ln(s, c, _(\" (use \\\"git rm --cached ...\\\" to unstage)\"));\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_dirty_header(struct wt_status *s,\n@@ -223,7 +223,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,\n \tstatus_printf_ln(s, c, _(\" (use \\\"git checkout -- ...\\\" to discard changes in working directory)\"));\n \tif (has_dirty_submodules)\n \t\tstatus_printf_ln(s, c, _(\" (commit or discard the untracked or modified content in submodules)\"));\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_other_header(struct wt_status *s,\n@@ -235,12 +235,12 @@ static void wt_status_print_other_header(struct wt_status *s,\n \tif (!s->hints)\n \t\treturn;\n \tstatus_printf_ln(s, c, _(\" (use \\\"git %s ...\\\" to include in what will be committed)\"), how);\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_trailer(struct wt_status *s)\n {\n-\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"\");\n+\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n }\n \n #define quote_path quote_path_relative\n@@ -826,7 +826,7 @@ static void wt_status_print_other(struct wt_status *s,\n \tstring_list_clear(&output, 0);\n \tstrbuf_release(&buf);\n conclude:\n-\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n+\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n }\n \n void wt_status_truncate_message_at_cut_line(struct strbuf *buf)\n@@ -913,7 +913,7 @@ static void wt_status_print_tracking(struct wt_status *s)\n \t\tcolor_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), \"%c\",\n \t\t\t\t comment_line_char);\n \telse\n-\t\tfprintf_ln(s->fp, \"\");\n+\t\tfputs(\"\", s->fp);\n }\n \n static int has_unmerged(struct wt_status *s)\n@@ -1329,7 +1329,7 @@ void wt_status_print(struct wt_status *s)\n \t\t\t\ton_what = _(\"Not currently on any branch.\");\n \t\t\t}\n \t\t}\n-\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t\tstatus_printf_more(s, branch_status_color, \"%s\", on_what);\n \t\tstatus_printf_more(s, branch_color, \"%s\\n\", branch_name);\n \t\tif (!s->is_initial)\n@@ -1342,9 +1342,9 @@ void wt_status_print(struct wt_status *s)\n \tfree(state.detached_from);\n \n \tif (s->is_initial) {\n-\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), _(\"Initial commit\"));\n-\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t}\n \n \twt_status_print_updated(s);\n@@ -1361,7 +1361,7 @@ void wt_status_print(struct wt_status *s)\n \t\tif (s->show_ignored_files)\n \t\t\twt_status_print_other(s, &s->ignored, _(\"Ignored files\"), \"add -f\");\n \t\tif (advice_status_u_option && 2000 < s->untracked_in_ms) {\n-\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n \t\t\t\t\t _(\"It took %.2f seconds to enumerate untracked files. 'status -uno'\\n\"\n \t\t\t\t\t \"may speed it up, but you have to be careful not to forget to add\\n\"\n-- \n1.9.2+fc1.20.g204a630\n"},{"id":"240696","messageId":"20140504190121.GP75770@vauxhall.crustytoothpaste.net","threadId":"36579","inReplyTo":"1399183975-2346-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-05-04T19:01:22Z","receivedAt":"2014-05-04T19:01:22Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:\n> This is in gcc 4.9.0:\n> \n> wt-status.c: In function ‘wt_status_print_unmerged_header’:\n> wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> status_printf_ln(s, c, \"\");\n> ^\n> \n> We could pass -Wno-format-zero-length, but it seems compiler-specific\n> flags are frowned upon, so let's just avoid the warning altogether.\n\nI believe these warnings existed before GCC 4.9 as well, but I'm not\nopposed to the change.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"240699","messageId":"53669f7ed2877_61fea2d30c62@nysa.notmuch","threadId":"36579","inReplyTo":"20140504190121.GP75770@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-04T20:13:50Z","receivedAt":"2014-05-04T20:13:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"brian m. carlson wrote:\n> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:\n> > This is in gcc 4.9.0:\n> > \n> > wt-status.c: In function ‘wt_status_print_unmerged_header’:\n> > wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> > status_printf_ln(s, c, \"\");\n> > ^\n> > \n> > We could pass -Wno-format-zero-length, but it seems compiler-specific\n> > flags are frowned upon, so let's just avoid the warning altogether.\n> \n> I believe these warnings existed before GCC 4.9 as well, but I'm not\n> opposed to the change.\n\nYes, I'm aware of that. I didn't say they started to happen in 4.9, I\nsaid they happen in 4.9.\n\n-- \nFelipe Contreras"},{"id":"240707","messageId":"20140505052117.GC6569@sigill.intra.peff.net","threadId":"36579","inReplyTo":"20140504190121.GP75770@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-05T05:21:17Z","receivedAt":"2014-05-05T05:21:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 04, 2014 at 07:01:22PM +0000, brian m. carlson wrote:\n\n> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:\n> > This is in gcc 4.9.0:\n> > \n> > wt-status.c: In function ‘wt_status_print_unmerged_header’:\n> > wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> > status_printf_ln(s, c, \"\");\n> > ^\n> > \n> > We could pass -Wno-format-zero-length, but it seems compiler-specific\n> > flags are frowned upon, so let's just avoid the warning altogether.\n> \n> I believe these warnings existed before GCC 4.9 as well, but I'm not\n> opposed to the change.\n\nYeah, this started last summer when we added __attribute__((format)) to\nthe status_printf_ln calls, and I posted essentially the same patch. We\nkind of waffled between \"eh, just set -Wno-format-zero-length\" and doing\nsomething, and ended up at the former. I'd be fine with doing it this\nway; we're not likely to add a lot of new callsites that would make it a\nhassle to keep up with.\n\n-Peff\n"},{"id":"240705","messageId":"5367257a857ce_2db613a731043@nysa.notmuch","threadId":"36579","inReplyTo":"20140505054901.GA19331@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-05T05:45:30Z","receivedAt":"2014-05-05T05:45:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:\n> \n> > So it looks like gcc is smarter now, and in trying to fix a few warnings\n> > we generated hundreds more.\n> > \n> > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.\n> \n> And now we've gone the other way, and re-enabled the initial warnings.\n> Can we come up with a solution that helps both cases?\n\nWhat initial warnings? As I explained already I don't get any warnings\nwith this patch series in gcc 4.9.0.\n\n-- \nFelipe Contreras\n"},{"id":"240717","messageId":"20140505054901.GA19331@sigill.intra.peff.net","threadId":"36579","inReplyTo":"1399183975-2346-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-05T05:49:01Z","receivedAt":"2014-05-05T05:49:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:\n\n> So it looks like gcc is smarter now, and in trying to fix a few warnings\n> we generated hundreds more.\n> \n> This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.\n\nAnd now we've gone the other way, and re-enabled the initial warnings.\nCan we come up with a solution that helps both cases?\n\nI could not find a way to annotate a value as \"maybe unused\", but we can\nhide it inside a function, like:\n\n-- >8 --\nSubject: inline error() function\n\nThe error() function does two things: it prints an error,\nand it returns \"-1\" as a convenience to allow:\n\n return error(\"foo\");\n\nCommit e208f9c converted this to a macro to make the\nconstant return value more visible to the compiler. However,\nrecent versions of gcc complain when error is used in a void\ncontext, as the constant \"-1\" ends up unused.\n\nInstead, let's convert error() to a static inline, which\nshould accomplish the same thing without the extra warnings\n(because gcc will not warn about unused return values unless\nwarn_unused_result is specified for the particular\nfunction).\n\nSigned-off-by: Jeff King \n---\n git-compat-util.h | 20 ++++++++++++--------\n usage.c | 8 +-------\n 2 files changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f6d3a46..3aef0d3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -310,7 +310,6 @@ extern NORETURN void usage(const char *err);\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n #ifndef NO_OPENSSL\n@@ -325,14 +324,19 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n \n /*\n * Let callers be aware of the constant return value; this can help\n- * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,\n- * because some compilers may not support variadic macros. Since we're only\n- * trying to help gcc, anyway, it's OK; other compilers will fall back to\n- * using the function as usual.\n+ * gcc with -Wuninitialized analysis.\n */\n-#if defined(__GNUC__) && ! defined(__clang__)\n-#define error(...) (error(__VA_ARGS__), -1)\n-#endif\n+__attribute__((format (printf, 1, 0)))\n+extern void error_impl(const char *err, va_list params);\n+__attribute__((format (printf, 1, 2)))\n+static inline int error(const char *err, ...)\n+{\n+ va_list params;\n+ va_start(params, err);\n+ error_impl(err, params);\n+ va_end(params);\n+ return -1;\n+}\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n extern void set_error_routine(void (*routine)(const char *err, va_list params));\ndiff --git a/usage.c b/usage.c\nindex ed14645..5456e4b 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -138,15 +138,9 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n-#undef error\n-int error(const char *err, ...)\n+void error_impl(const char *err, va_list params)\n {\n-\tva_list params;\n-\n-\tva_start(params, err);\n \terror_routine(err, params);\n-\tva_end(params);\n-\treturn -1;\n }\n \n void warning(const char *warn, ...)\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"240724","messageId":"20140505060202.GA27360@sigill.intra.peff.net","threadId":"36579","inReplyTo":"5367257a857ce_2db613a731043@nysa.notmuch","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-05T06:02:02Z","receivedAt":"2014-05-05T06:02:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 05, 2014 at 12:45:30AM -0500, Felipe Contreras wrote:\n\n> Jeff King wrote:\n> > On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:\n> > \n> > > So it looks like gcc is smarter now, and in trying to fix a few warnings\n> > > we generated hundreds more.\n> > > \n> > > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.\n> > \n> > And now we've gone the other way, and re-enabled the initial warnings.\n> > Can we come up with a solution that helps both cases?\n> \n> What initial warnings? As I explained already I don't get any warnings\n> with this patch series in gcc 4.9.0.\n\nThe \"few warnings\" in your statement quoted above.\n\nYou could try reading the commit message of the commit you are\nreverting, which explains it, but the short answer is: try compiling\nwith -O3.\n\n-Peff\n"},{"id":"240721","messageId":"53672c53f2ed9_2e86acd30c6e@nysa.notmuch","threadId":"36579","inReplyTo":"20140505060202.GA27360@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-05T06:14:43Z","receivedAt":"2014-05-05T06:14:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Mon, May 05, 2014 at 12:45:30AM -0500, Felipe Contreras wrote:\n> \n> > Jeff King wrote:\n> > > On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:\n> > > \n> > > > So it looks like gcc is smarter now, and in trying to fix a few warnings\n> > > > we generated hundreds more.\n> > > > \n> > > > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.\n> > > \n> > > And now we've gone the other way, and re-enabled the initial warnings.\n> > > Can we come up with a solution that helps both cases?\n> > \n> > What initial warnings? As I explained already I don't get any warnings\n> > with this patch series in gcc 4.9.0.\n> \n> The \"few warnings\" in your statement quoted above.\n> \n> You could try reading the commit message of the commit you are\n> reverting, which explains it, but the short answer is: try compiling\n> with -O3.\n\nSigh. And I'm the one with the abrasive style of communication.\n\n-- \nFelipe Contreras\n"},{"id":"240711","messageId":"20140505062931.GA9311@sigill.intra.peff.net","threadId":"36579","inReplyTo":"53672c53f2ed9_2e86acd30c6e@nysa.notmuch","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-05T06:29:31Z","receivedAt":"2014-05-05T06:29:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 05, 2014 at 01:14:43AM -0500, Felipe Contreras wrote:\n\n> Jeff King wrote:\n> > On Mon, May 05, 2014 at 12:45:30AM -0500, Felipe Contreras wrote:\n> > \n> > > Jeff King wrote:\n> > > > On Sun, May 04, 2014 at 01:12:53AM -0500, Felipe Contreras wrote:\n> > > > \n> > > > > So it looks like gcc is smarter now, and in trying to fix a few warnings\n> > > > > we generated hundreds more.\n> > > > > \n> > > > > This reverts commit e208f9cc7574f5980faba498d0aa30b4defeb34f.\n> > > > \n> > > > And now we've gone the other way, and re-enabled the initial warnings.\n> > > > Can we come up with a solution that helps both cases?\n> > > \n> > > What initial warnings? As I explained already I don't get any warnings\n> > > with this patch series in gcc 4.9.0.\n> > \n> > The \"few warnings\" in your statement quoted above.\n> > \n> > You could try reading the commit message of the commit you are\n> > reverting, which explains it, but the short answer is: try compiling\n> > with -O3.\n> \n> Sigh. And I'm the one with the abrasive style of communication.\n\nI apologize if that seemed abrasive. I am slightly annoyed that you\nseemed to be reverting my commit without understanding (or dealing with)\nthe problem that the original fixed.\n\nBut I was _also_ trying to point you in the right direction by directing\nyou to -O3. Do you see the problem now? And did you look at the\nfollow-up patch I sent?\n\n-Peff\n"},{"id":"240710","messageId":"53673e0499ebe_78fd70f2ecb5@nysa.notmuch","threadId":"36579","inReplyTo":"20140505062931.GA9311@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-05T07:30:12Z","receivedAt":"2014-05-05T07:30:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Mon, May 05, 2014 at 01:14:43AM -0500, Felipe Contreras wrote:\n> > Jeff King wrote:\n\n> > > You could try reading the commit message of the commit you are\n> > > reverting, which explains it, but the short answer is: try compiling\n> > > with -O3.\n> > \n> > Sigh. And I'm the one with the abrasive style of communication.\n> \n> I apologize if that seemed abrasive. I am slightly annoyed that you\n> seemed to be reverting my commit without understanding (or dealing with)\n> the problem that the original fixed.\n\nThe original problem happens only with -O3, I tried only with -O2 and\ndidn't see any problems.\n\nIf we have a) code that fixes a couple warnings with -O3 but introduces\nhundreds with -O2, vs. b) code that has only a comple warnings with -O3,\nI'd go for b) any day.\n\n> But I was _also_ trying to point you in the right direction by directing\n> you to -O3. Do you see the problem now? And did you look at the\n> follow-up patch I sent?\n\nYes, I see the problem now with -O3. And yes, I looked at the patch you\nsent. I haven't tested it but I bet it would sove both problems.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"240725","messageId":"20140505212938.GA16715@sigill.intra.peff.net","threadId":"36579","inReplyTo":"53673e0499ebe_78fd70f2ecb5@nysa.notmuch","subject":"Re: [PATCH 1/3] Revert \"make error()'s constant return value more visible\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-05T21:29:39Z","receivedAt":"2014-05-05T21:29:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 05, 2014 at 02:30:12AM -0500, Felipe Contreras wrote:\n\n> If we have a) code that fixes a couple warnings with -O3 but introduces\n> hundreds with -O2, vs. b) code that has only a comple warnings with -O3,\n> I'd go for b) any day.\n\nI agree. But my point was to ask whether we can fix both.\n\n> Yes, I see the problem now with -O3. And yes, I looked at the patch you\n> sent. I haven't tested it but I bet it would sove both problems.\n\nUnfortunately, it does not work in all cases (and I obviously did\nsomething wrong when testing it last night). I should have taken my own\nadvice and re-read the commit message for e208f9c more carefully, which\nsays:\n\n If we can make the compiler aware that error() will always\n return -1, it can do a better job of analysis. The simplest\n method would be to inline the error() function. However,\n this doesn't work, because gcc will not inline a variadc\n function. We can work around this by defining a macro.[...]\n\nI cannot think of any other way to make the compiler aware of the\nconstant value, but perhaps somebody else is more clever than I am.\nAnother alternative is to write out \"return error(...)\" as \"error(...);\nreturn -1\". In some ways that is more readable, though it is more\nverbose (and would cause quite a bit of code churn).\n\nSo applying your patches may be the least-bad solution.\n\n-Peff\n"},{"id":"240792","messageId":"20140506151441.GA25768@sigill.intra.peff.net","threadId":"36579","inReplyTo":"20140505212938.GA16715@sigill.intra.peff.net","subject":"[PATCH 1/2] inline constant return from error() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-06T15:14:42Z","receivedAt":"2014-05-06T15:14:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit e208f9c introduced a macro to turn error() calls\ninto:\n\n (error(), -1)\n\nto make the constant return value more visible to the\ncalling code (and thus let the compiler make better\ndecisions about the code).\n\nThis works well for code like:\n\n return error(...);\n\nbut the \"-1\" is superfluous in code that just calls error()\nwithout caring about the return value. In older versions of\ngcc, that was fine, but gcc 4.9 complains with -Wunused-value.\n\nWe can work around this by encapsulating the constant return\nvalue in a static inline function, as gcc specifically\navoids complaining about unused function returns unless the\nfunction has been specifically marked with the\nwarn_unused_result attribute.\n\nWe also use the same trick for config_error_nonbool and\nopterror, which learned the same error technique in a469a10.\n\nReported-by: Felipe Contreras \nSigned-off-by: Jeff King \n---\nOn Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:\n\n> I cannot think of any other way to make the compiler aware of the\n> constant value, but perhaps somebody else is more clever than I am.\n\nThis came to me in a dream, and seems to work.\n\n cache.h | 2 +-\n git-compat-util.h | 6 +++++-\n parse-options.h | 2 +-\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..e2f12b0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1272,7 +1272,7 @@ extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n #if defined(__GNUC__) && ! defined(__clang__)\n-#define config_error_nonbool(s) (config_error_nonbool(s), -1)\n+#define config_error_nonbool(s) (config_error_nonbool(s), const_error())\n #endif\n extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f6d3a46..b4c437e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n * using the function as usual.\n */\n #if defined(__GNUC__) && ! defined(__clang__)\n-#define error(...) (error(__VA_ARGS__), -1)\n+static inline int const_error(void)\n+{\n+\treturn -1;\n+}\n+#define error(...) (error(__VA_ARGS__), const_error())\n #endif\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\ndiff --git a/parse-options.h b/parse-options.h\nindex 3189676..2f9be96 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -177,7 +177,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n extern int optbug(const struct option *opt, const char *reason);\n extern int opterror(const struct option *opt, const char *reason, int flags);\n #if defined(__GNUC__) && ! defined(__clang__)\n-#define opterror(o,r,f) (opterror((o),(r),(f)), -1)\n+#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())\n #endif\n \n /*----- incremental advanced APIs -----*/\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"240787","messageId":"20140506151750.GB25768@sigill.intra.peff.net","threadId":"36579","inReplyTo":"20140505212938.GA16715@sigill.intra.peff.net","subject":"[PATCH 2/2] let clang use the constant-return error() macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-06T15:17:50Z","receivedAt":"2014-05-06T15:17:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit e208f9c converted error() into a macro to make its\nconstant return value more apparent to calling code. Commit\n5ded807 prevents us using this macro with clang, since\nclang's -Wunused-value is smart enough to realize that the\nconstant \"-1\" is useless in some contexts.\n\nHowever, since the last commit puts the constant behind an\ninline function call, this is enough to prevent the\n-Wunused-value warning on both modern gcc and clang. So we\ncan now re-enable the macro when compiling with clang.\n\nTested with clang 3.3, 3.4, and 3.5.\n\nSigned-off-by: Jeff King \n---\nI still get warnings when compiling with clang -O3, due to\n-Warray-bounds. It looks like a bug, though. Clang complains that:\n\n strcmp(argv[1], \"git\")\n\noversteps array bounds when the strcmp is expanded into a mess of\nbuiltin magic. So I don't think we are doing anything wrong here.\n\n cache.h | 2 +-\n git-compat-util.h | 2 +-\n parse-options.h | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e2f12b0..35a3e6b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1271,7 +1271,7 @@ extern int check_repository_format_version(const char *var, const char *value, v\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n-#if defined(__GNUC__) && ! defined(__clang__)\n+#if defined(__GNUC__)\n #define config_error_nonbool(s) (config_error_nonbool(s), const_error())\n #endif\n extern const char *get_log_output_encoding(void);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b4c437e..70dc028 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -330,7 +330,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n * trying to help gcc, anyway, it's OK; other compilers will fall back to\n * using the function as usual.\n */\n-#if defined(__GNUC__) && ! defined(__clang__)\n+#if defined(__GNUC__)\n static inline int const_error(void)\n {\n \treturn -1;\ndiff --git a/parse-options.h b/parse-options.h\nindex 2f9be96..7940bc7 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -176,7 +176,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n \n extern int optbug(const struct option *opt, const char *reason);\n extern int opterror(const struct option *opt, const char *reason, int flags);\n-#if defined(__GNUC__) && ! defined(__clang__)\n+#if defined(__GNUC__)\n #define opterror(o,r,f) (opterror((o),(r),(f)), const_error())\n #endif\n \n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"240848","messageId":"xmqqppjqczhq.fsf@gitster.dls.corp.google.com","threadId":"36579","inReplyTo":"20140506151441.GA25768@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-06T22:29:37Z","receivedAt":"2014-05-06T22:29:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Commit e208f9c introduced a macro to turn error() calls\n> into:\n>\n> (error(), -1)\n>\n> to make the constant return value more visible to the\n> calling code (and thus let the compiler make better\n> decisions about the code).\n>\n> This works well for code like:\n>\n> return error(...);\n>\n> but the \"-1\" is superfluous in code that just calls error()\n> without caring about the return value. In older versions of\n> gcc, that was fine, but gcc 4.9 complains with -Wunused-value.\n>\n> We can work around this by encapsulating the constant return\n> value in a static inline function, as gcc specifically\n> avoids complaining about unused function returns unless the\n> function has been specifically marked with the\n> warn_unused_result attribute.\n\nThat's kind of W*A*T magic, and I generally try to avoid magic, as\nlong as it solves your \"can we make both -O2 with new compilers and\n-O3 happy?\" I wouldn't complain ;-)\n\n> We also use the same trick for config_error_nonbool and\n> opterror, which learned the same error technique in a469a10.\n>\n> Reported-by: Felipe Contreras \n> Signed-off-by: Jeff King \n> ---\n> On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:\n>\n>> I cannot think of any other way to make the compiler aware of the\n>> constant value, but perhaps somebody else is more clever than I am.\n>\n> This came to me in a dream, and seems to work.\n\n\n\n>\n> cache.h | 2 +-\n> git-compat-util.h | 6 +++++-\n> parse-options.h | 2 +-\n> 3 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 107ac61..e2f12b0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1272,7 +1272,7 @@ extern int git_env_bool(const char *, int);\n> extern int git_config_system(void);\n> extern int config_error_nonbool(const char *);\n> #if defined(__GNUC__) && ! defined(__clang__)\n> -#define config_error_nonbool(s) (config_error_nonbool(s), -1)\n> +#define config_error_nonbool(s) (config_error_nonbool(s), const_error())\n> #endif\n> extern const char *get_log_output_encoding(void);\n> extern const char *get_commit_output_encoding(void);\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index f6d3a46..b4c437e 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n> * using the function as usual.\n> */\n> #if defined(__GNUC__) && ! defined(__clang__)\n> -#define error(...) (error(__VA_ARGS__), -1)\n> +static inline int const_error(void)\n> +{\n> +\treturn -1;\n> +}\n> +#define error(...) (error(__VA_ARGS__), const_error())\n> #endif\n> \n> extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n> diff --git a/parse-options.h b/parse-options.h\n> index 3189676..2f9be96 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -177,7 +177,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n> extern int optbug(const struct option *opt, const char *reason);\n> extern int opterror(const struct option *opt, const char *reason, int flags);\n> #if defined(__GNUC__) && ! defined(__clang__)\n> -#define opterror(o,r,f) (opterror((o),(r),(f)), -1)\n> +#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())\n> #endif\n> \n> /*----- incremental advanced APIs -----*/\n"},{"id":"240867","messageId":"20140507030254.GC20726@sigill.intra.peff.net","threadId":"36579","inReplyTo":"xmqqppjqczhq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-07T03:02:54Z","receivedAt":"2014-05-07T03:02:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 06, 2014 at 03:29:37PM -0700, Junio C Hamano wrote:\n\n> > We can work around this by encapsulating the constant return\n> > value in a static inline function, as gcc specifically\n> > avoids complaining about unused function returns unless the\n> > function has been specifically marked with the\n> > warn_unused_result attribute.\n> \n> That's kind of W*A*T magic, and I generally try to avoid magic, as\n> long as it solves your \"can we make both -O2 with new compilers and\n> -O3 happy?\" I wouldn't complain ;-)\n\nI agree it's rather magical, but I think it's something we can count on.\nCertainly turning on warn_unused_result for every function would be a\ncatastrophe for most code bases, and I don't expect gcc to do it. It's\npossible it would eventually grow smart to say \"eh, I inlined this and\nrealized that you don't use the return value\", but I think that would be\nsimilarly a bad idea.\n\nAnd it does work with -O2 and -O3 with both gcc-4.9 and clang in my\ntests.\n\n-Peff\n"},{"id":"240930","messageId":"xmqq4n11bgf6.fsf@gitster.dls.corp.google.com","threadId":"36579","inReplyTo":"20140505052117.GC6569@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-07T18:19:09Z","receivedAt":"2014-05-07T18:19:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, May 04, 2014 at 07:01:22PM +0000, brian m. carlson wrote:\n>\n>> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:\n>> > This is in gcc 4.9.0:\n>> > \n>> > wt-status.c: In function ‘wt_status_print_unmerged_header’:\n>> > wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n>> > status_printf_ln(s, c, \"\");\n>> > ^\n>> > \n>> > We could pass -Wno-format-zero-length, but it seems compiler-specific\n>> > flags are frowned upon, so let's just avoid the warning altogether.\n>> \n>> I believe these warnings existed before GCC 4.9 as well, but I'm not\n>> opposed to the change.\n>\n> Yeah, this started last summer when we added __attribute__((format)) to\n> the status_printf_ln calls, and I posted essentially the same patch. We\n> kind of waffled between \"eh, just set -Wno-format-zero-length\" and doing\n> something, and ended up at the former. I'd be fine with doing it this\n> way; we're not likely to add a lot of new callsites that would make it a\n> hassle to keep up with.\n\nOK, so I'll take it as your Ack ;-)\n"},{"id":"240942","messageId":"20140507200542.GC4013@sandbox-ub","threadId":"36579","inReplyTo":"xmqq4n11bgf6.fsf@gitster.dls.corp.google.com","subject":"Re: Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2014-05-07T20:05:42Z","receivedAt":"2014-05-07T20:05:42Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, May 07, 2014 at 11:19:09AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Sun, May 04, 2014 at 07:01:22PM +0000, brian m. carlson wrote:\n> >\n> >> On Sun, May 04, 2014 at 01:12:55AM -0500, Felipe Contreras wrote:\n> >> > This is in gcc 4.9.0:\n> >> > \n> >> > wt-status.c: In function ‘wt_status_print_unmerged_header’:\n> >> > wt-status.c:191:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> >> > status_printf_ln(s, c, \"\");\n> >> > ^\n> >> > \n> >> > We could pass -Wno-format-zero-length, but it seems compiler-specific\n> >> > flags are frowned upon, so let's just avoid the warning altogether.\n> >> \n> >> I believe these warnings existed before GCC 4.9 as well, but I'm not\n> >> opposed to the change.\n> >\n> > Yeah, this started last summer when we added __attribute__((format)) to\n> > the status_printf_ln calls, and I posted essentially the same patch. We\n> > kind of waffled between \"eh, just set -Wno-format-zero-length\" and doing\n> > something, and ended up at the former. I'd be fine with doing it this\n> > way; we're not likely to add a lot of new callsites that would make it a\n> > hassle to keep up with.\n> \n> OK, so I'll take it as your Ack ;-)\n\nWhat happened to this patch? These warnings are still annoying me on my\nUbuntu 14.04.\n\nCheers Heiko\n"},{"id":"240948","messageId":"xmqqr4458h6b.fsf@gitster.dls.corp.google.com","threadId":"36579","inReplyTo":"20140507200542.GC4013@sandbox-ub","subject":"Re: [PATCH 3/3] Silence a bunch of format-zero-length warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-07T20:31:08Z","receivedAt":"2014-05-07T20:31:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> On Wed, May 07, 2014 at 11:19:09AM -0700, Junio C Hamano wrote:\n>> Jeff King writes:\n>> ...\n>> > Yeah, this started last summer when we added __attribute__((format)) to\n>> > the status_printf_ln calls, and I posted essentially the same patch. We\n>> > kind of waffled between \"eh, just set -Wno-format-zero-length\" and doing\n>> > something, and ended up at the former. I'd be fine with doing it this\n>> > way; we're not likely to add a lot of new callsites that would make it a\n>> > hassle to keep up with.\n>> \n>> OK, so I'll take it as your Ack ;-)\n>\n> What happened to this patch? These warnings are still annoying me on my\n> Ubuntu 14.04.\n\nAs Peff summarized, an earlier patch was dropped with \"It is easy\nfor builders with such compilers to squelch the warning.\" We are\nreversing the course, with an updated log message like this ;-)\n\n The user have long been told to pass -Wno-format-zero-length, but a\n patch that avoids warning altogether is not too noisy, so let's do\n so.\n"},{"id":"241260","messageId":"536f22fd76455_6535d01308cb@nysa.notmuch","threadId":"36579","inReplyTo":"xmqqppjqczhq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-11T07:13:01Z","receivedAt":"2014-05-11T07:13:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> That's kind of W*A*T magic, and I generally try to avoid magic, as\n> long as it solves your \"can we make both -O2 with new compilers and\n> -O3 happy?\" I wouldn't complain ;-)\n\nIn case anybody is looking for a non-hacky way of doing this that\ndoesn't depend on the gcc version of the week, this is how I silenced\nthe warnings in git-fc:\n\nhttps://github.com/felipec/git/commit/e14ca39ba0092f0305d5fb347e20b829f736b29b\n\n-- \nFelipe Contreras\n"},{"id":"241262","messageId":"xmqqppjkut6s.fsf@gitster.dls.corp.google.com","threadId":"36579","inReplyTo":"20140507030254.GC20726@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-11T17:22:03Z","receivedAt":"2014-05-11T17:22:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 06, 2014 at 03:29:37PM -0700, Junio C Hamano wrote:\n>\n>> That's kind of W*A*T magic, and I generally try to avoid magic, as\n>> long as it solves your \"can we make both -O2 with new compilers and\n>> -O3 happy?\" I wouldn't complain ;-)\n>\n> I agree it's rather magical, but I think it's something we can count on.\n\nCertainly. Sorry that I missed \"but\" before \"as long as\", which made\nme sound as if I were unhappy. At least, I didn't call it an ugly\n\"hack\" ;-)\n\nThe alternative you mentioned up-thread \"... to write out \"return\nerror(...)\" as \"error(...); return -1\". In some ways that is more\nreadable, though it is more verbose...\" has one more downside you\ndid not mention, and the approach to encapsulate it inside error()\nwill not have it: new call-sites to error() do not have to worry\nabout the issue with this approach.\n\nUntil it breaks, that is. But that goes without saying with the\n\"it's something we can count on\" pre-condition in place ;-).\n"},{"id":"241301","messageId":"20140512181326.GA31164@sigill.intra.peff.net","threadId":"36579","inReplyTo":"xmqqppjkut6s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-12T18:13:26Z","receivedAt":"2014-05-12T18:13:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 11, 2014 at 10:22:03AM -0700, Junio C Hamano wrote:\n\n> The alternative you mentioned up-thread \"... to write out \"return\n> error(...)\" as \"error(...); return -1\". In some ways that is more\n> readable, though it is more verbose...\" has one more downside you\n> did not mention, and the approach to encapsulate it inside error()\n> will not have it: new call-sites to error() do not have to worry\n> about the issue with this approach.\n> \n> Until it breaks, that is. But that goes without saying with the\n> \"it's something we can count on\" pre-condition in place ;-).\n\nYeah, I agree with this thinking. I'd rather not do something that\nimpacts each callsite until we have exhausted other options that hide\nthe complexity in the definition.\n\n-Peff\n"},{"id":"241303","messageId":"20140512184426.GO9218@google.com","threadId":"36579","inReplyTo":"20140506151441.GA25768@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-05-12T18:44:26Z","receivedAt":"2014-05-12T18:44:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:\n\n>> I cannot think of any other way to make the compiler aware of the\n>> constant value, but perhaps somebody else is more clever than I am.\n>\n> This came to me in a dream, and seems to work.\n\nClever. :) Thanks for thinking it up.\n\n[...]\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n> * using the function as usual.\n> */\n> #if defined(__GNUC__) && ! defined(__clang__)\n> -#define error(...) (error(__VA_ARGS__), -1)\n> +static inline int const_error(void)\n> +{\n> +\treturn -1;\n> +}\n> +#define error(...) (error(__VA_ARGS__), const_error())\n\nI wish we could just make error() an inline function that calls some\nprint_error() helper, but I don't believe all compilers used to build\ngit are smart enough to inline uses of varargs. So this looks like\nthe right thing to do.\n\nI kind of wish we weren't in so much of an arms race with static\nanalyzers. Is there some standard way to submit our code as \"an idiom\nthat should continue not to produce warnings\" to be included in a\ntestsuite and prevent problems in the future?\n\nThanks,\nJonathan\n"},{"id":"241307","messageId":"20140512185645.GA32569@sigill.intra.peff.net","threadId":"36579","inReplyTo":"20140512184426.GO9218@google.com","subject":"Re: [PATCH 1/2] inline constant return from error() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-12T18:56:45Z","receivedAt":"2014-05-12T18:56:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 12, 2014 at 11:44:26AM -0700, Jonathan Nieder wrote:\n\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -331,7 +331,11 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))\n> > * using the function as usual.\n> > */\n> > #if defined(__GNUC__) && ! defined(__clang__)\n> > -#define error(...) (error(__VA_ARGS__), -1)\n> > +static inline int const_error(void)\n> > +{\n> > +\treturn -1;\n> > +}\n> > +#define error(...) (error(__VA_ARGS__), const_error())\n> \n> I wish we could just make error() an inline function that calls some\n> print_error() helper, but I don't believe all compilers used to build\n> git are smart enough to inline uses of varargs. So this looks like\n> the right thing to do.\n\nNot just \"not all compilers\", but \"not even gcc\". If you declare it\nalways_inline, gcc will even complain \"you can't do that, it has\nvarargs\".\n\nFor my curiosity, is there any compiler which _does_ inline varargs\ncalls? Clang would be the obvious guess.\n\n> I kind of wish we weren't in so much of an arms race with static\n> analyzers. Is there some standard way to submit our code as \"an idiom\n> that should continue not to produce warnings\" to be included in a\n> testsuite and prevent problems in the future?\n\nI agree the arms race is frustrating, but I think the compiler is being\ncompletely reasonable in this case. It has flagged code where it knows\na variable may be uninitialized depending on the return value from\nerror(). The only reason I as a human know it's a false positive is that\nI know error() will always return -1. So it's not an idiom here, so much\nas letting the compiler know everything we know.\n\nIt would just be nice if there was an easier way to do it.\n\nI do think giving the compiler that information is nicer than an\nattribute saying \"trust me, it's initialized\". The constant return not\nonly silences the warnings, but it may actually let the compiler produce\nbetter code (both in these cases, but in the hundreds of other spots\nthat call error()).\n\n-Peff\n"}]}