git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH/RFC 2/3] inline error functions with constant returns

From
Jeff King <peff@peff.net>
Date
Dec 14, 2012, 22:12 UTC
Message-ID
<20121214221235.GB19677@sigill.intra.peff.net>
In-Reply-To
<20121214220903.GA18418@sigill.intra.peff.net>

The error() function reports an error message to stderr and returns -1. That makes it handy for returning errors from functions with a single-line:

  return error("something went wrong", ...);

In this case, we know something that the compiler does not, namely that this is equivalent to:

  error("something went wrong", ...);
  return -1;

Knowing that the return value is constant can let the compiler do better control flow analysis, which means it can give more accurate answers for static warnings, like -Wuninitialized. But because error() is found in a different compilation unit, the compiler doesn't get to see the code when making decisions about the caller.

This patch makes error(), along with a handful of functions which wrap it, an inline function, giving the compiler the extra information. This prevents some false positives when -Wunitialized is used with -O3.

Signed-off-by: Jeff King <peff@peff.net>
---
Not really meant for inclusion.  The opterror() bit does silence one
warning, but I think the error() inlining is doing absolutely nothing.
 cache.h           | 10 +++++++++-
 config.c          |  9 ---------
 git-compat-util.h | 13 ++++++++++++-
 parse-options.c   | 11 ++++++-----
 parse-options.h   |  8 +++++++-
 usage.c           | 12 +-----------
 6 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/cache.h b/cache.h
index 18fdd18..fb7c5e2 100644
--- a/cache.h
+++ b/cache.h
@@ -1135,10 +1135,18 @@ extern const char *get_commit_output_encoding(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
-extern int config_error_nonbool(const char *);
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
+/*
+ * Call this to report error for your variable that should not
+ * get a boolean value (i.e. "[my] var" means "true").
+ */
+static inline int config_error_nonbool(const char *var)
+{
+	return error("Missing value for '%s'", var);
+}
+
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
 
 struct config_include_data {
diff --git a/config.c b/config.c
index fb3f868..ea4a98f 100644
--- a/config.c
+++ b/config.c
@@ -1655,12 +1655,3 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 {
 	return git_config_rename_section_in_file(NULL, old_name, new_name);
 }
-
-/*
- * Call this to report error for your variable that should not
- * get a boolean value (i.e. "[my] var" means "true").
- */
-int config_error_nonbool(const char *var)
-{
-	return error("Missing value for '%s'", var);
-}
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..c38de42 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -285,9 +285,20 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
 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)));
 
+extern void (*error_routine)(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_routine(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/parse-options.c b/parse-options.c
index c1c66bd..5268d4e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -18,13 +18,14 @@ int opterror(const struct option *opt, const char *reason, int flags)
 	return error("BUG: switch '%c' %s", opt->short_name, reason);
 }
 
-int opterror(const struct option *opt, const char *reason, int flags)
+void opterror_report(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);
+		error("switch `%c' %s", opt->short_name, reason);
+	else if (flags & OPT_UNSET)
+		error("option `no-%s' %s", opt->long_name, reason);
+	else
+		error("option `%s' %s", opt->long_name, reason);
 }
 
 static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..23673c7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -176,7 +176,13 @@ extern int optbug(const struct option *opt, const char *reason);
 				   const struct option *options);
 
 extern int optbug(const struct option *opt, const char *reason);
-extern int opterror(const struct option *opt, const char *reason, int flags);
+extern void opterror_report(const struct option *opt, const char *reason, int flags);
+static inline int opterror(const struct option *opt, const char *reason, int flags)
+{
+	opterror_report(opt, reason, flags);
+	return -1;
+}
+
 /*----- incremental advanced APIs -----*/
 
 enum {
diff --git a/usage.c b/usage.c
index 8eab281..9f8e342 100644
--- a/usage.c
+++ b/usage.c
@@ -53,7 +53,7 @@ static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_b
  * (ugh), so keep things static. */
 static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;
 static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
+void (*error_routine)(const char *err, va_list params) = error_builtin;
 static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 
 void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))
@@ -130,16 +130,6 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
-int error(const char *err, ...)
-{
-	va_list params;
-
-	va_start(params, err);
-	error_routine(err, params);
-	va_end(params);
-	return -1;
-}
-
 void warning(const char *warn, ...)
 {
 	va_list params;

-- 
1.8.0.2.4.g59402aa
Previous: Florian AchleitnerNext: Jeff King
Message 4 of 12 in “compiling git with gcc -O3 -Wuninitialized”
  1. 0/3 compiling git with gcc -O3 -WuninitializedJeff King, Dec 14, 2012
  2. 1/3 remote-testsvn: fix unitialized variableJeff King, Dec 14, 2012
  3. Florian AchleitnerDec 15, 2012
  4. 2/3 inline error functions with constant returnsJeff King, Dec 14, 2012
  5. 3/3 silence some -Wuninitialized warnings around errorsJeff King, Dec 14, 2012
  6. Nguyen Thai Ngoc DuyDec 15, 2012
  7. Jeff KingDec 15, 2012
  8. Johannes SixtDec 15, 2012
  9. Jeff KingDec 15, 2012
  10. 0/2 compiling git with gcc -O3 -WuninitializedJeff King, Dec 15, 2012
  11. 1/2 make error()'s constant return value more visibleJeff King, Dec 15, 2012
  12. 2/2 silence some -Wuninitialized false positivesJeff King, Dec 15, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.