threads / patch / 36112

patch, 2 partsfix status_printf_ln calls "zero-length format" warnings

Subject: [PATCH 0/2] fix status_printf_ln calls "zero-length format" warnings

## tl;dr

6 messages between Mar 10, 2014 and Mar 11, 2014. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Benoit Pierre· Mar 10, 2014, 19:27 UTC · lore

Those happens with "gcc -Wformat-zero-length". Since passing NULL does not generate a warning (as __attribute__((printf())) does not imply nonull), modify status_printf/status_printf_ln to allow a NULL format and update calls with an empty string.

Benoit Pierre (2):
  status: allow NULL fmt for status_printf/status_vprintf_ln
  fix status_printf_ln calls "zero-length format" warnings

 builtin/commit.c |  2 +-
 wt-status.c      | 23 ++++++++++++-----------
 2 files changed, 13 insertions(+), 12 deletions(-)

-- 
1.9.0

Benoit Pierre· Mar 10, 2014, 19:27 UTC · re: Benoit Pierre · lore

[PATCH 1/2] status: allow NULL fmt for status_printf/status_vprintf_ln

Useful for calling status_printf only to change/reset the color (and output an additional '\n' with status_vprintf_ln).

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 wt-status.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Show changes to wt-status.c +2 −1

diff --git a/wt-status.c b/wt-status.c
index 4e55810..17f63a4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -49,7 +49,8 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 	struct strbuf linebuf = STRBUF_INIT;
 	const char *line, *eol;
 
-	strbuf_vaddf(&sb, fmt, ap);
+	if (NULL != fmt)
+	    strbuf_vaddf(&sb, fmt, ap);
 	if (!sb.len) {
 		if (s->display_comment_prefix) {
 			strbuf_addch(&sb, comment_line_char);

-- 
1.9.0

Eric Sunshine· Mar 10, 2014, 20:13 UTC · re: Benoit Pierre · lore

Re: [PATCH 1/2] status: allow NULL fmt for status_printf/status_vprintf_ln

On Mon, Mar 10, 2014 at 3:27 PM, Benoit Pierre <benoit.pierre@gmail.com> wrote:

Show 18 quoted lines

> Useful for calling status_printf only to change/reset the color (and
> output an additional '\n' with status_vprintf_ln).
>
> Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
> ---
>  wt-status.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 4e55810..17f63a4 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -49,7 +49,8 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
>         struct strbuf linebuf = STRBUF_INIT;
>         const char *line, *eol;
>
> -       strbuf_vaddf(&sb, fmt, ap);
> +       if (NULL != fmt)

In this codebase,

    if (fmt)

would be more idiomatic.

Show 6 quoted lines

> +           strbuf_vaddf(&sb, fmt, ap);
>         if (!sb.len) {
>                 if (s->display_comment_prefix) {
>                         strbuf_addch(&sb, comment_line_char);
> --
> 1.9.0

Benoit Pierre· Mar 10, 2014, 19:27 UTC · re: Benoit Pierre · lore

[PATCH 2/2] fix status_printf_ln calls "zero-length format" warnings

Those happens with "gcc -Wformat-zero-length".

Change empty strings to NULL now that it's allowed.

Signed-off-by: Benoit Pierre <benoit.pierre@gmail.com>
---
 builtin/commit.c |  2 +-
 wt-status.c      | 20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

Show changes to 2 files +11 −11

builtin/commit.c, wt-status.c

diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..6d19439 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -805,7 +805,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, NULL);
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
diff --git a/wt-status.c b/wt-status.c
index 17f63a4..d9b1d00 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -189,7 +189,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, NULL);
 }
 
 static void wt_status_print_cached_header(struct wt_status *s)
@@ -205,7 +205,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, NULL);
 }
 
 static void wt_status_print_dirty_header(struct wt_status *s,
@@ -224,7 +224,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, NULL);
 }
 
 static void wt_status_print_other_header(struct wt_status *s,
@@ -236,12 +236,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, NULL);
 }
 
 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), NULL);
 }
 
 #define quote_path quote_path_relative
@@ -794,7 +794,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, NULL);
 }
 
 void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
@@ -1292,7 +1292,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), NULL);
 		status_printf_more(s, branch_status_color, "%s", on_what);
 		status_printf_more(s, branch_color, "%s\n", branch_name);
 		if (!s->is_initial)
@@ -1305,9 +1305,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), NULL);
 		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), NULL);
 	}
 
 	wt_status_print_updated(s);
@@ -1324,7 +1324,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, NULL);
 			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.0

Jeff King· Mar 10, 2014, 19:51 UTC · re: Benoit Pierre · lore

Re: [PATCH 0/2] fix status_printf_ln calls "zero-length format" warnings

On Mon, Mar 10, 2014 at 08:27:25PM +0100, Benoit Pierre wrote:

> Those happens with "gcc -Wformat-zero-length". Since passing NULL does not
> generate a warning (as __attribute__((printf())) does not imply nonull), modify
> status_printf/status_printf_ln to allow a NULL format and update calls with an
> empty string.

Most of us who compile with -Wall decided a while ago to use -Wno-format-zero-length, because it really is a silly complaint (it assumes there are no side effects of the function besides printing the format string, which is obviously not true in this case).

It would be nice to compile out of the box with "-Wall -Werror", and I think your solution looks relatively clean. But I am slightly concerned about the assumption that it is OK to pass a NULL fmt parameter to a function marked with __attribute__((format)). It certainly seems to be the case now, and I do not know of any plans for that to change, but it seems like a potentially obvious thing for the compiler to check.

I dunno; perhaps it has already been considered and rejected by gcc folks to allow the exact escape hatch we are using here.

-Peff

Junio C Hamano· Mar 11, 2014, 19:25 UTC · re: Jeff King · lore

Re: [PATCH 0/2] fix status_printf_ln calls "zero-length format" warnings

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> Most of us who compile with -Wall decided a while ago to use
> -Wno-format-zero-length, because it really is a silly complaint (it
> assumes there are no side effects of the function besides printing the
> format string, which is obviously not true in this case).
>
> It would be nice to compile out of the box with "-Wall -Werror", and I
> think your solution looks relatively clean. But I am slightly concerned
> about the assumption that it is OK to pass a NULL fmt parameter to a
> function marked with __attribute__((format)).  It certainly seems to be
> the case now, and I do not know of any plans for that to change, but it
> seems like a potentially obvious thing for the compiler to check.

Yes, exactly.