{"thread":{"id":"34495","subject":"[PATCH] status_printf_ln: Suppress false positive warnings of empty format string.","startedAt":"2013-07-19T14:08:09Z","lastAt":"2013-07-19T16:04:28Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"223761","messageId":"1374242889-14239-1-git-send-email-stefanbeller@googlemail.com","threadId":"34495","inReplyTo":null,"subject":"[PATCH] status_printf_ln: Suppress false positive warnings of empty format string.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-19T14:08:09Z","receivedAt":"2013-07-19T14:08:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a response to 8dd0ee823f1829a3aa228c3c73e31de5c89b5317.\n\nInstead of having an empty string as format for the printf like function\nstatus_printf_ln, we could insert an empty string into the format\nparameter.\n\nA similar fixup commit is found in linux (2e4c332913b5), but there\nthe empty string is replaced by a string containing one whitespace.\n\nTo determine, which approach is better I setup 2 test programs, which\neither have a whitespace format (\" \") or the empty string (\"%s\", \"\"),\nlooking like:\n-\n\t#include <stdlib.h>\n\t#include <stdio.h>\n\tint main (int argc, char** argv) {\n\t\tlong i;\n\t\tfor (i = 0; i < 1024*1024*1024; ++i)\n\t\t\tprintf(\" \");\n\t}\n-\nChecking the required time of the programs, while redirecting the actual\noutput (the billion white spaces compared to nothing) to /dev/null\nindicates that the approach used in this patch is faster regardless\nof the optimization level of gcc.\n\nAlso this patch doesn't change output, which favors this approach over\nthe whitespace approach.\n\nThe only thing left to discuss, whether this patch is worth it, as it\nonly suppresses false positive warnings from gcc, but makes the\ncode slightly harder to read.\n\nSigned-off-by: Stefan Beller <stefanbeller@googlemail.com>\n---\n builtin/commit.c |  2 +-\n wt-status.c      | 18 +++++++++---------\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 65cf2a7..34bc274 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -773,7 +773,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 cb24f1f..912ed88 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -179,7 +179,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 <file>...\\\" 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@@ -195,7 +195,7 @@ static void wt_status_print_cached_header(struct wt_status *s)\n \t\tstatus_printf_ln(s, c, _(\"  (use \\\"git reset %s <file>...\\\" to unstage)\"), s->reference);\n \telse\n \t\tstatus_printf_ln(s, c, _(\"  (use \\\"git rm --cached <file>...\\\" 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@@ -214,7 +214,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,\n \tstatus_printf_ln(s, c, _(\"  (use \\\"git checkout -- <file>...\\\" 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@@ -226,12 +226,12 @@ static void wt_status_print_other_header(struct wt_status *s,\n \tif (!advice_status_hints)\n \t\treturn;\n \tstatus_printf_ln(s, c, _(\"  (use \\\"git %s <file>...\\\" 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@@ -1192,7 +1192,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@@ -1205,9 +1205,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@@ -1224,7 +1224,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.8.3.3.754.g9c3c367.dirty\n"},{"id":"223775","messageId":"7v7ggmy1wn.fsf@alter.siamese.dyndns.org","threadId":"34495","inReplyTo":"1374242889-14239-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH] status_printf_ln: Suppress false positive warnings of empty format string.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T16:01:28Z","receivedAt":"2013-07-19T16:01:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <stefanbeller@googlemail.com> writes:\n\n> This is a response to 8dd0ee823f1829a3aa228c3c73e31de5c89b5317.\n>\n> Instead of having an empty string as format for the printf like function\n> status_printf_ln, we could insert an empty string into the format\n> parameter.\n>\n> A similar fixup commit is found in linux (2e4c332913b5), but there\n> the empty string is replaced by a string containing one whitespace.\n>\n> To determine, which approach is better I setup 2 test programs, which\n> either have a whitespace format (\" \") or the empty string (\"%s\", \"\"),\n> looking like:\n> -\n> \t#include <stdlib.h>\n> \t#include <stdio.h>\n> \tint main (int argc, char** argv) {\n> \t\tlong i;\n> \t\tfor (i = 0; i < 1024*1024*1024; ++i)\n> \t\t\tprintf(\" \");\n> \t}\n> -\n> Checking the required time of the programs, while redirecting the actual\n> output (the billion white spaces compared to nothing) to /dev/null\n> indicates that the approach used in this patch is faster regardless\n> of the optimization level of gcc.\n>\n> Also this patch doesn't change output, which favors this approach over\n> the whitespace approach.\n\nEven if the \" \" variant is faster, it does not matter if its output\nis incorrect ;-)\n\n> The only thing left to discuss, whether this patch is worth it, as it\n> only suppresses false positive warnings from gcc, but makes the\n> code slightly harder to read.\n\nI think we discussed this already?  The conclusion was it is silly\nfor GCC to warn on -Wformat-zero-length for user-defined function in\nthe first place, IIRC.  func(other, args, fmt,...), when invoked as\nfunc(other, args, \"\"), may very well do something useful regardless\nof the formatting part based on other args.\n\nFor that matter, I personally think -Wformat-zero-length warning for\nstandard ones, like\n\n\tprintf(\"\");\n\nis silly, too.  It is not like\n\n\tprintf(\"\", extra, arg);\n\nis not caught as an error.\n\nSo we can just add -Wno-format-zero-length after -Wall and be done\nwith it?\n\n>\n> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>\n> ---\n>  builtin/commit.c |  2 +-\n>  wt-status.c      | 18 +++++++++---------\n>  2 files changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 65cf2a7..34bc274 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -773,7 +773,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;\n> diff --git a/wt-status.c b/wt-status.c\n> index cb24f1f..912ed88 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -179,7 +179,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 <file>...\\\" 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> @@ -195,7 +195,7 @@ static void wt_status_print_cached_header(struct wt_status *s)\n>  \t\tstatus_printf_ln(s, c, _(\"  (use \\\"git reset %s <file>...\\\" to unstage)\"), s->reference);\n>  \telse\n>  \t\tstatus_printf_ln(s, c, _(\"  (use \\\"git rm --cached <file>...\\\" 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> @@ -214,7 +214,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,\n>  \tstatus_printf_ln(s, c, _(\"  (use \\\"git checkout -- <file>...\\\" 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> @@ -226,12 +226,12 @@ static void wt_status_print_other_header(struct wt_status *s,\n>  \tif (!advice_status_hints)\n>  \t\treturn;\n>  \tstatus_printf_ln(s, c, _(\"  (use \\\"git %s <file>...\\\" 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> @@ -1192,7 +1192,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> @@ -1205,9 +1205,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> @@ -1224,7 +1224,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"},{"id":"223776","messageId":"51E9638C.9090207@googlemail.com","threadId":"34495","inReplyTo":"7v7ggmy1wn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] status_printf_ln: Suppress false positive warnings of empty format string.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-19T16:04:28Z","receivedAt":"2013-07-19T16:04:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"\n> \n> Even if the \" \" variant is faster, it does not matter if its output\n> is incorrect ;-)\n\nAgreed.\n\n> I think we discussed this already?  The conclusion was it is silly\n> for GCC to warn on -Wformat-zero-length for user-defined function in\n> the first place, IIRC.  func(other, args, fmt,...), when invoked as\n> func(other, args, \"\"), may very well do something useful regardless\n> of the formatting part based on other args.\n> \n\nI am sorry, I did not pay attention enough to that specific topic. I\nwas just finding warnings on next and pu, so I wondered how to fix it.\n\n\n\n"}]}