Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly
Jeff King p...@peff.net writes: On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote: config.c:#undef config_error_nonbool config.c:int config_error_nonbool(const char *var) You could always look in the commit history: $ git log -S'#define config_error_nonbool' cache.h or search the mailing list: http://thread.gmane.org/gmane.comp.version-control.git/211505 Presumably this was done so that the uses of config_error_nonbool can be recognized as returning -1 unconditionally. Yes, it helps prevent false positives in gcc's flow analysis (specifically, -Wuninitialized warnings). But is that worth the obfuscation? Yes? gcc's flow analysis works with the same data as humans reading the code. If there is no information content in the function call, it makes more sense to either making it void. One can always explicitly write (config_error_nonbool(panic-when-assailed), -1) for shortcircuit use inside of an expression even when config_error_nonbool is a function returning a void expression: comma operators do not try type coercion on their first argument. Shrug. This one has likely been discussed to death already. Sometimes it's more convenient to avoid getting a question asked in the first place rather than having a stock answer for it. -- David Kastrup -- 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/5] notes-utils: handle boolean notes.rewritemode correctly
On Tue, Feb 18, 2014 at 09:41:51AM +0100, David Kastrup wrote:
gcc's flow analysis works with the same data as humans reading the
code. If there is no information content in the function call, it makes
more sense to either making it void.
The point of error() returning a constant -1 is to use this idiom:
if (something_failed)
return error(this will get printed, and we get a -1 return);
From a code perspective it's pointless. You could just write:
if (something_failed) {
error(...);
return -1;
}
which is equivalent. But the point is that the former is shorter and a
little more readable, assuming you are familiar with the idiom.
One can always explicitly write
(config_error_nonbool(panic-when-assailed), -1)
Yes, but again, the point is readability. Doing that at each callsite is
ugly and annoying.
Shrug. This one has likely been discussed to death already. Sometimes
it's more convenient to avoid getting a question asked in the first
place rather than having a stock answer for it.
You are the first person to ask about it, so there is no stock answer.
However, everything I told you was in the commit messages and the list
archive already. We can also avoid questions being asked by using those
tools.
-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/5] notes-utils: handle boolean notes.rewritemode correctly
Jeff King p...@peff.net writes:
On Tue, Feb 18, 2014 at 09:41:51AM +0100, David Kastrup wrote:
gcc's flow analysis works with the same data as humans reading the
code. If there is no information content in the function call, it makes
more sense to either making it void.
The point of error() returning a constant -1 is to use this idiom:
if (something_failed)
return error(this will get printed, and we get a -1 return);
if (something_failed)
return error(this will get printed), -1;
Not much of an idiom, no insider logic hidden to both compiler and
reader.
From a code perspective it's pointless. You could just write:
if (something_failed) {
error(...);
return -1;
}
which is equivalent. But the point is that the former is shorter and a
little more readable, assuming you are familiar with the idiom.
Assuming that.
One can always explicitly write
(config_error_nonbool(panic-when-assailed), -1)
Yes, but again, the point is readability. Doing that at each callsite is
ugly and annoying.
I consider insider semantics ugly and annoying. To each his own.
You are the first person to ask about it, so there is no stock answer.
However, everything I told you was in the commit messages and the list
archive already. We can also avoid questions being asked by using
those tools.
It's further raising the barriers for contributors if they are expected
to have studied the last few years in the mailing archive. And the
skills needed for that kind of study are mostly unrelated to good
programming skills.
So I am less than convinced that this is among the coding practices that
can be expected to attract/scare away potential contributors in
proportion to their expected capability of advancing/hindering the
project.
It's not me running the shop, so it's nothing more than an opinion.
--
David Kastrup
--
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/5] notes-utils: handle boolean notes.rewritemode correctly
On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote: Not really relevant to this patch, but looking at the output of git grep config_error_nonbool seems like a serious amount of ridiculousness going on. The header shows cache.h:extern int config_error_nonbool(const char *); cache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1) and the implementation config.c:#undef config_error_nonbool config.c:int config_error_nonbool(const char *var) You could always look in the commit history: $ git log -S'#define config_error_nonbool' cache.h or search the mailing list: http://thread.gmane.org/gmane.comp.version-control.git/211505 Presumably this was done so that the uses of config_error_nonbool can be recognized as returning -1 unconditionally. Yes, it helps prevent false positives in gcc's flow analysis (specifically, -Wuninitialized warnings). But is that worth the obfuscation? Yes? Why not let config_error_nonbool return -1 in the first place? It originally did, but callers do not get to see the static return, so they miss some analysis opportunities. It does not appear like any caller would call the function rather than the macro, so why declare the function as returning an int at all? Because we don't use these macros on non-gnu compilers; they actually see the real function return. And why give it the same name as the macro (risking human/computer confusion and requiring an explicit #undef for the definition or was that declaration?) instead of config_error_nonbool_internal or whatever else? This particular case is simple, but see error() for another use of the same technique which requires variadic macros, which are not available everywhere. Callers want to say just error(...), so we have to name the macro that. If we call the matching function error_internal, then non-gnu compilers would need a macro to convert error(...) to error_internal(...). But they can't do so, because it would be a variadic macro. So you could adjust config_error_nonbool(), but not error(). But that introduces its own confusion, because the technique is not applied consistently. I agree the #define is ugly. But it's confined to a small area, and it does solve an actual problem. -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
[PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly
If we carry on after outputting config_error_nonbool then we're
guaranteed to dereference a null pointer.
Signed-off-by: John Keeping j...@keeping.me.uk
---
notes-utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/notes-utils.c b/notes-utils.c
index 2975dcd..4aa7023 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -75,7 +75,7 @@ static int notes_rewrite_config(const char *k, const char *v,
void *cb)
return 0;
} else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
if (!v)
- config_error_nonbool(k);
+ return config_error_nonbool(k);
c-combine = parse_combine_notes_fn(v);
if (!c-combine) {
error(_(Bad notes.rewriteMode value: '%s'), v);
--
1.9.rc0.187.g6292fff
--
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/5] notes-utils: handle boolean notes.rewritemode correctly
John Keeping j...@keeping.me.uk writes: If we carry on after outputting config_error_nonbool then we're guaranteed to dereference a null pointer. Not really relevant to this patch, but looking at the output of git grep config_error_nonbool seems like a serious amount of ridiculousness going on. The header shows cache.h:extern int config_error_nonbool(const char *); cache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1) and the implementation config.c:#undef config_error_nonbool config.c:int config_error_nonbool(const char *var) Presumably this was done so that the uses of config_error_nonbool can be recognized as returning -1 unconditionally. But is that worth the obfuscation? Why not let config_error_nonbool return -1 in the first place? It does not appear like any caller would call the function rather than the macro, so why declare the function as returning an int at all? And why give it the same name as the macro (risking human/computer confusion and requiring an explicit #undef for the definition or was that declaration?) instead of config_error_nonbool_internal or whatever else? -- David Kastrup -- 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
