git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Sixt <j6t@kdbg.org>
Cc: git@vger.kernel.org
Subject: [PATCH 2/2] silence some -Wuninitialized false positives
Date: Sat, 15 Dec 2012 12:42:10 -0500	[thread overview]
Message-ID: <20121215174210.GB22069@sigill.intra.peff.net> (raw)
In-Reply-To: <20121215173621.GA21011@sigill.intra.peff.net>

There are a few error functions that simply wrap error() and
provide a standardized message text. Like error(), they
always return -1; knowing that can help the compiler silence
some false positive -Wuninitialized warnings.

One strategy would be to just declare these as inline in the
header file so that the compiler can see that they always
return -1. However, gcc does not always inline them (e.g.,
it will not inline opterror, even with -O3), which renders
our change pointless.

Instead, let's follow the same route we did with error() in
the last patch, and define a macro that makes the constant
return value obvious to the compiler.

Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to force inlining with
__attribute(always_inline)__.  But I don't like that, as we are
affecting the generated code in that case (and any time we are
overriding gcc's decision, I have to assume that it is smarter about
when to inline than we are).

Other variants include:

  1. Inline functions, but keep them as one-liners. E.g.:

     int opterror(...)
     {
            real_opterror(...);
            return -1;
     }

  2. Using these macros even when __GNUC__ isn't set. Unlike the
     variadic error() macro, these do not use any special features.
     If we used them everywhere, the functions themselves could be
     converted to a void return. That would make it less likely that
     somebody modifying the function in the future would fail to realize
     that the error return must always be -1.

I dunno. All the solutions are a bit ugly. I really do like being -Wall
clean, but I wonder if this is spending too much effort to work around
the compiler (we could also just mark these few cases as "int foo =
foo" to say we have manually verified that they're OK).

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

diff --git a/cache.h b/cache.h
index 18fdd18..0e8e5d8 100644
--- a/cache.h
+++ b/cache.h
@@ -1136,6 +1136,9 @@ extern int config_error_nonbool(const char *);
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int config_error_nonbool(const char *);
+#ifdef __GNUC__
+#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 fb3f868..526f682 100644
--- a/config.c
+++ b/config.c
@@ -1660,6 +1660,7 @@ 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 c1c66bd..67e98a6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -18,15 +18,6 @@ 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)
 {
@@ -594,3 +585,12 @@ 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 71a39c6..e703853 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,6 +177,10 @@ extern int opterror(const struct option *opt, const char *reason, int flags);
 
 extern int optbug(const struct option *opt, const char *reason);
 extern int opterror(const struct option *opt, const char *reason, int flags);
+#ifdef __GNUC__
+#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
+#endif
+
 /*----- incremental advanced APIs -----*/
 
 enum {
-- 
1.8.0.2.4.g59402aa

      parent reply	other threads:[~2012-12-15 17:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-14 22:09 [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized Jeff King
2012-12-14 22:11 ` [PATCH 1/3] remote-testsvn: fix unitialized variable Jeff King
2012-12-15 10:29   ` Florian Achleitner
2012-12-14 22:12 ` [PATCH/RFC 2/3] inline error functions with constant returns Jeff King
2012-12-14 22:13 ` [PATCH/RFC 3/3] silence some -Wuninitialized warnings around errors Jeff King
2012-12-15  3:07 ` [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized Nguyen Thai Ngoc Duy
2012-12-15 10:09   ` Jeff King
2012-12-15 10:49 ` Johannes Sixt
2012-12-15 11:09   ` Jeff King
2012-12-15 17:36     ` [PATCH/RFCv2 0/2] " Jeff King
2012-12-15 17:37       ` [PATCH 1/2] make error()'s constant return value more visible Jeff King
2012-12-15 17:42       ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121215174210.GB22069@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).