Re: What's cooking in git.git (Dec 2012, #05; Tue, 18)
On Tue, Dec 18, 2012 at 12:31:43PM -0800, Junio C Hamano wrote: * jk/error-const-return (2012-12-15) 2 commits - silence some -Wuninitialized false positives - make error()'s constant return value more visible Help compilers' flow analysis by making it more explicit that error() always returns -1, to reduce false variable used uninitialized warnings. This is still an RFC. What's your opinion on this? The last round I posted (and what you have in pu) is about as good as this technique is going to get. So it is really a taste thing. On the one hand, I really like that it is not just papering over the problem, but rather giving the compiler more data to do its analysis. On the other hand, it makes the source kind of ugly. I don't think there's a rush on it, but I'm just sifting through my half-done topics. -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: What's cooking in git.git (Dec 2012, #05; Tue, 18)
Jeff King p...@peff.net writes: On Tue, Dec 18, 2012 at 12:31:43PM -0800, Junio C Hamano wrote: * jk/error-const-return (2012-12-15) 2 commits - silence some -Wuninitialized false positives - make error()'s constant return value more visible Help compilers' flow analysis by making it more explicit that error() always returns -1, to reduce false variable used uninitialized warnings. This is still an RFC. What's your opinion on this? Ugly but not too much so, and it would be useful. The only thing that makes it ugly is that it promises error() will return -1 and nothing else forever, but at this point in the evolution of the codebase, I think we are pretty much committed to it anyway, so I do not think it is a problem. -- 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: What's cooking in git.git (Dec 2012, #05; Tue, 18)
On Thu, Dec 20, 2012 at 10:13:37AM -0800, Junio C Hamano wrote:
* jk/error-const-return (2012-12-15) 2 commits
- silence some -Wuninitialized false positives
- make error()'s constant return value more visible
Help compilers' flow analysis by making it more explicit that
error() always returns -1, to reduce false variable used
uninitialized warnings.
This is still an RFC.
What's your opinion on this?
Ugly but not too much so, and it would be useful.
The only thing that makes it ugly is that it promises error() will
return -1 and nothing else forever, but at this point in the
evolution of the codebase, I think we are pretty much committed to
it anyway, so I do not think it is a problem.
Right. I do not mind saying error() will always return -1 and forcing
somebody who changes that assumption to update the macro. But what
worries me is that when they make that update, there is no compile-time
check that indicates the macro and the function are no longer in sync.
So our attempt for more compile-time safety may actually introduce a
run-time bug. And it is a hard bug to find, as the preprocessor
magically converts the error code into -1 without you being able to
see it in the code.
It would be safer to just unconditionally use the macros and drop the
return values from the functions entirely, like the patch below
(squashed on top of what is in jk/error-const-return). But it cannot
work for error(), because the variadic nature means we need to restrict
ourselves to __GNUC__.
diff --git a/cache.h b/cache.h
index 0e8e5d8..694b146 100644
--- a/cache.h
+++ b/cache.h
@@ -1135,10 +1135,8 @@ extern int config_error_nonbool(const char *);
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 *);
-#ifdef __GNUC__
+extern void config_error_nonbool(const char *);
#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 526f682..a22e78c 100644
--- a/config.c
+++ b/config.c
@@ -1661,7 +1661,7 @@ int config_error_nonbool(const char *var)
* get a boolean value (i.e. [my] var means true).
*/
#undef config_error_nonbool
-int config_error_nonbool(const char *var)
+void config_error_nonbool(const char *var)
{
- return error(Missing value for '%s', var);
+ error(Missing value for '%s', var);
}
diff --git a/parse-options.c b/parse-options.c
index 67e98a6..ba39dd9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -586,11 +586,12 @@ int opterror(const struct option *opt, const char
*reason, int flags)
}
#undef opterror
-int opterror(const struct option *opt, const char *reason, int flags)
+void opterror(const struct option *opt, const char *reason, int flags)
{
if (flags OPT_SHORT)
- return error(switch `%c' %s, opt-short_name, reason);
+ 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(option `no-%s' %s, opt-long_name, reason);
+ else
+ error(option `%s' %s, opt-long_name, reason);
}
diff --git a/parse-options.h b/parse-options.h
index e703853..bd43314 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -176,10 +176,8 @@ extern int opterror(const struct option *opt, const char
*reason, int flags);
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);
-#ifdef __GNUC__
+extern void opterror(const struct option *opt, const char *reason, int flags);
#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
-#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: What's cooking in git.git (Dec 2012, #05; Tue, 18)
Jeff King p...@peff.net writes:
So our attempt for more compile-time safety may actually introduce a
run-time bug. And it is a hard bug to find, as the preprocessor
magically converts the error code into -1 without you being able to
see it in the code.
It would be safer to just unconditionally use the macros and drop the
return values from the functions entirely, like the patch below
(squashed on top of what is in jk/error-const-return). But it cannot
work for error(), because the variadic nature means we need to restrict
ourselves to __GNUC__.
While I like the general direction, I am not sure if we are OK with
the introduction of
#undef config_error_nonbool
-int config_error_nonbool(const char *var)
+void config_error_nonbool(const char *var)
{
- return error(Missing value for '%s', var);
+ error(Missing value for '%s', var);
}
a new, arguably false, returned value not used warning from this
point. Perhaps it is fine for now, as we have tons of calls that do
not cast the return value to (void).
--
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: What's cooking in git.git (Dec 2012, #05; Tue, 18)
Junio C Hamano gits...@pobox.com writes: Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The tip of the 'master' branch is a bit past 1.8.1-rc2; hopefully we can go final around the end of the week. Many topics are getting into good shape in 'next', hopefully ready to be merged soon after the 1.8.1 final. As we seem to be getting more minor documentation updates that are safe for the upcoming release, and also because I've updated the AsciiDoc toolchain used to prepare the preformatted manpage and HTML tarballs, I changed my mind to delay the final til the end of the year, and tag an rc3 toward the end of this week instead. Both repositories http://code.google.com/p/git-htmldocs/ and http://code.google.com/p/git-manpages/, and the browser-reachable pages at http://git-htmldocs.googlecode.com/git/git.html should start getting the output formatted with AsciiDoc 8.6.8. The following documentation updates topics are likely to be in the rc3: as/doc-for-devs cr/doc-checkout-branch jc/doc-diff-blobs jc/fetch-tags-doc jk/avoid-mailto-invalid-in-doc nd/index-format-doc sl/git-svn-docs sl/readme-gplv2 ta/api-index-doc Also I am planning to have the .mailmap cleanup topic in the rc3: jk/mailmap-cleanup Thanks. -- 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
