Re: [PATCH 1/2] inline constant return from error() function
On Sun, May 11, 2014 at 10:22:03AM -0700, Junio C Hamano wrote: The alternative you mentioned up-thread ... to write out return error(...) as error(...); return -1. In some ways that is more readable, though it is more verbose... has one more downside you did not mention, and the approach to encapsulate it inside error() will not have it: new call-sites to error() do not have to worry about the issue with this approach. Until it breaks, that is. But that goes without saying with the it's something we can count on pre-condition in place ;-). Yeah, I agree with this thinking. I'd rather not do something that impacts each callsite until we have exhausted other options that hide the complexity in the definition. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] inline constant return from error() function
Hi,
Jeff King wrote:
On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:
I cannot think of any other way to make the compiler aware of the
constant value, but perhaps somebody else is more clever than I am.
This came to me in a dream, and seems to work.
Clever. :) Thanks for thinking it up.
[...]
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,11 @@ extern void warning(const char *err, ...)
__attribute__((format (printf, 1, 2)))
* using the function as usual.
*/
#if defined(__GNUC__) ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
+static inline int const_error(void)
+{
+ return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
I wish we could just make error() an inline function that calls some
print_error() helper, but I don't believe all compilers used to build
git are smart enough to inline uses of varargs. So this looks like
the right thing to do.
I kind of wish we weren't in so much of an arms race with static
analyzers. Is there some standard way to submit our code as an idiom
that should continue not to produce warnings to be included in a
testsuite and prevent problems in the future?
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] inline constant return from error() function
On Mon, May 12, 2014 at 11:44:26AM -0700, Jonathan Nieder wrote:
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,11 @@ extern void warning(const char *err, ...)
__attribute__((format (printf, 1, 2)))
* using the function as usual.
*/
#if defined(__GNUC__) ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
+static inline int const_error(void)
+{
+ return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
I wish we could just make error() an inline function that calls some
print_error() helper, but I don't believe all compilers used to build
git are smart enough to inline uses of varargs. So this looks like
the right thing to do.
Not just not all compilers, but not even gcc. If you declare it
always_inline, gcc will even complain you can't do that, it has
varargs.
For my curiosity, is there any compiler which _does_ inline varargs
calls? Clang would be the obvious guess.
I kind of wish we weren't in so much of an arms race with static
analyzers. Is there some standard way to submit our code as an idiom
that should continue not to produce warnings to be included in a
testsuite and prevent problems in the future?
I agree the arms race is frustrating, but I think the compiler is being
completely reasonable in this case. It has flagged code where it knows
a variable may be uninitialized depending on the return value from
error(). The only reason I as a human know it's a false positive is that
I know error() will always return -1. So it's not an idiom here, so much
as letting the compiler know everything we know.
It would just be nice if there was an easier way to do it.
I do think giving the compiler that information is nicer than an
attribute saying trust me, it's initialized. The constant return not
only silences the warnings, but it may actually let the compiler produce
better code (both in these cases, but in the hundreds of other spots
that call error()).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] inline constant return from error() function
Junio C Hamano wrote: That's kind of W*A*T magic, and I generally try to avoid magic, as long as it solves your can we make both -O2 with new compilers and -O3 happy? I wouldn't complain ;-) In case anybody is looking for a non-hacky way of doing this that doesn't depend on the gcc version of the week, this is how I silenced the warnings in git-fc: https://github.com/felipec/git/commit/e14ca39ba0092f0305d5fb347e20b829f736b29b -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] inline constant return from error() function
Jeff King p...@peff.net writes: On Tue, May 06, 2014 at 03:29:37PM -0700, Junio C Hamano wrote: That's kind of W*A*T magic, and I generally try to avoid magic, as long as it solves your can we make both -O2 with new compilers and -O3 happy? I wouldn't complain ;-) I agree it's rather magical, but I think it's something we can count on. Certainly. Sorry that I missed but before as long as, which made me sound as if I were unhappy. At least, I didn't call it an ugly hack ;-) The alternative you mentioned up-thread ... to write out return error(...) as error(...); return -1. In some ways that is more readable, though it is more verbose... has one more downside you did not mention, and the approach to encapsulate it inside error() will not have it: new call-sites to error() do not have to worry about the issue with this approach. Until it breaks, that is. But that goes without saying with the it's something we can count on pre-condition in place ;-). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] inline constant return from error() function
Commit e208f9c introduced a macro to turn error() calls
into:
(error(), -1)
to make the constant return value more visible to the
calling code (and thus let the compiler make better
decisions about the code).
This works well for code like:
return error(...);
but the -1 is superfluous in code that just calls error()
without caring about the return value. In older versions of
gcc, that was fine, but gcc 4.9 complains with -Wunused-value.
We can work around this by encapsulating the constant return
value in a static inline function, as gcc specifically
avoids complaining about unused function returns unless the
function has been specifically marked with the
warn_unused_result attribute.
We also use the same trick for config_error_nonbool and
opterror, which learned the same error technique in a469a10.
Reported-by: Felipe Contreras felipe.contre...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:
I cannot think of any other way to make the compiler aware of the
constant value, but perhaps somebody else is more clever than I am.
This came to me in a dream, and seems to work.
cache.h | 2 +-
git-compat-util.h | 6 +-
parse-options.h | 2 +-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 107ac61..e2f12b0 100644
--- a/cache.h
+++ b/cache.h
@@ -1272,7 +1272,7 @@ extern int git_env_bool(const char *, int);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
#if defined(__GNUC__) ! defined(__clang__)
-#define config_error_nonbool(s) (config_error_nonbool(s), -1)
+#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..b4c437e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,11 @@ extern void warning(const char *err, ...)
__attribute__((format (printf, 1, 2)))
* using the function as usual.
*/
#if defined(__GNUC__) ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
+static inline int const_error(void)
+{
+ return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
#endif
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err,
va_list params));
diff --git a/parse-options.h b/parse-options.h
index 3189676..2f9be96 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,7 +177,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
extern int optbug(const struct option *opt, const char *reason);
extern int opterror(const struct option *opt, const char *reason, int flags);
#if defined(__GNUC__) ! defined(__clang__)
-#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
+#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
#endif
/*- incremental advanced APIs -*/
--
2.0.0.rc1.436.g03cb729
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] inline constant return from error() function
Jeff King p...@peff.net writes:
Commit e208f9c introduced a macro to turn error() calls
into:
(error(), -1)
to make the constant return value more visible to the
calling code (and thus let the compiler make better
decisions about the code).
This works well for code like:
return error(...);
but the -1 is superfluous in code that just calls error()
without caring about the return value. In older versions of
gcc, that was fine, but gcc 4.9 complains with -Wunused-value.
We can work around this by encapsulating the constant return
value in a static inline function, as gcc specifically
avoids complaining about unused function returns unless the
function has been specifically marked with the
warn_unused_result attribute.
That's kind of W*A*T magic, and I generally try to avoid magic, as
long as it solves your can we make both -O2 with new compilers and
-O3 happy? I wouldn't complain ;-)
We also use the same trick for config_error_nonbool and
opterror, which learned the same error technique in a469a10.
Reported-by: Felipe Contreras felipe.contre...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
On Mon, May 05, 2014 at 05:29:38PM -0400, Jeff King wrote:
I cannot think of any other way to make the compiler aware of the
constant value, but perhaps somebody else is more clever than I am.
This came to me in a dream, and seems to work.
cache.h | 2 +-
git-compat-util.h | 6 +-
parse-options.h | 2 +-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 107ac61..e2f12b0 100644
--- a/cache.h
+++ b/cache.h
@@ -1272,7 +1272,7 @@ extern int git_env_bool(const char *, int);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
#if defined(__GNUC__) ! defined(__clang__)
-#define config_error_nonbool(s) (config_error_nonbool(s), -1)
+#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
#endif
extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..b4c437e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,7 +331,11 @@ extern void warning(const char *err, ...)
__attribute__((format (printf, 1, 2)))
* using the function as usual.
*/
#if defined(__GNUC__) ! defined(__clang__)
-#define error(...) (error(__VA_ARGS__), -1)
+static inline int const_error(void)
+{
+ return -1;
+}
+#define error(...) (error(__VA_ARGS__), const_error())
#endif
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err,
va_list params));
diff --git a/parse-options.h b/parse-options.h
index 3189676..2f9be96 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,7 +177,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
extern int optbug(const struct option *opt, const char *reason);
extern int opterror(const struct option *opt, const char *reason, int flags);
#if defined(__GNUC__) ! defined(__clang__)
-#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
+#define opterror(o,r,f) (opterror((o),(r),(f)), const_error())
#endif
/*- incremental advanced APIs -*/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] inline constant return from error() function
On Tue, May 06, 2014 at 03:29:37PM -0700, Junio C Hamano wrote: We can work around this by encapsulating the constant return value in a static inline function, as gcc specifically avoids complaining about unused function returns unless the function has been specifically marked with the warn_unused_result attribute. That's kind of W*A*T magic, and I generally try to avoid magic, as long as it solves your can we make both -O2 with new compilers and -O3 happy? I wouldn't complain ;-) I agree it's rather magical, but I think it's something we can count on. Certainly turning on warn_unused_result for every function would be a catastrophe for most code bases, and I don't expect gcc to do it. It's possible it would eventually grow smart to say eh, I inlined this and realized that you don't use the return value, but I think that would be similarly a bad idea. And it does work with -O2 and -O3 with both gcc-4.9 and clang in my tests. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
