refs.c: use skip_prefix() in prettify_refname()
PATCH 1/1 refs.c: use skip_prefix() in prettify_refname()
This eliminates three magic numbers.
--- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
refs.c +5 -5
@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)const char *prettify_refname (const char *name) { -return name + ( -starts_with (name,"refs/heads/" ) ?11 : -starts_with (name,"refs/tags/" ) ?10 : -starts_with (name,"refs/remotes/" ) ?13 : -0 ); +if (skip_prefix (name,"refs/heads/" , &name) || +skip_prefix (name,"refs/tags/" , &name) || +skip_prefix (name,"refs/remotes/" , &name)) + ;/* nothing */ +return name; }static const char *ref_rev_parse_rules[] = {
Signature · 1 line
2.12.1.485.g1616aa492
Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
Quoted 15 lines
This eliminates three magic numbers.
Signed-off-by SZEDER Gábor <[email protected]>--- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)refs.c +0
-0@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)const char *prettify_refname (const char *name) {
refs.c int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data) · line 373
Quoted 10 lines
refs.c +5
-5-return name + ( -starts_with (name,"refs/heads/" ) ?11 : -starts_with (name,"refs/tags/" ) ?10 : -starts_with (name,"refs/remotes/" ) ?13 : -0 ); +if (skip_prefix (name,"refs/heads/" , &name) || +skip_prefix (name,"refs/tags/" , &name) || +skip_prefix (name,"refs/remotes/" , &name)) + ;/* nothing */ +return name;
Nice, but why add the "if" when it's doing nothing?
René
Quoted 14 lines · René Scharfe
Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
Quoted 13 lines
This eliminates three magic numbers.
Signed-off-by SZEDER Gábor <[email protected]>--- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)refs.c +0
-0@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
Quoted 14 lines
refs.c int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data) · line 373
Quoted 12 lines
refs.c +5
-5const char *prettify_refname (const char *name) { -return name + ( -starts_with (name,"refs/heads/" ) ?11 : -starts_with (name,"refs/tags/" ) ?10 : -starts_with (name,"refs/remotes/" ) ?13 : -0 ); +if (skip_prefix (name,"refs/heads/" , &name) || +skip_prefix (name,"refs/tags/" , &name) || +skip_prefix (name,"refs/remotes/" , &name)) + ;/* nothing */ +return name;Nice, but why add the "if" when it's doing nothing?
It's short-circuiting in the conditional.
-Peff
Quoted 31 lines · Jeff King
Quoted 14 lines · René Scharfe
Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
Quoted 13 lines
This eliminates three magic numbers.
Signed-off-by SZEDER Gábor <[email protected]>--- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)refs.c +0
-0@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)Quoted 14 lines
refs.c int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data) · line 373
Quoted 12 lines
refs.c +5
-5const char *prettify_refname (const char *name) { -return name + ( -starts_with (name,"refs/heads/" ) ?11 : -starts_with (name,"refs/tags/" ) ?10 : -starts_with (name,"refs/remotes/" ) ?13 : -0 ); +if (skip_prefix (name,"refs/heads/" , &name) || +skip_prefix (name,"refs/tags/" , &name) || +skip_prefix (name,"refs/remotes/" , &name)) + ;/* nothing */ +return name;Nice, but why add the "if" when it's doing nothing?
Quoted 1 line
It's short-circuiting in the conditional.
You do not need a conditional to short circuit things. || works outside an if, too. ;)
Anyway, maybe it's worth spelling it out with an if .. else if cascade for readability?
After your comment of short-circuiting this code is pretty clear, so maybe just a small comment would do?
Thanks,
Stefan
Quoted 2 lines
-Peff
Quoted 31 lines · Jeff King
Quoted 14 lines · René Scharfe
Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
Quoted 13 lines
This eliminates three magic numbers.
Signed-off-by SZEDER Gábor <[email protected]>--- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)refs.c +0
-0@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)Quoted 14 lines
refs.c int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data) · line 373
Quoted 12 lines
refs.c +5
-5const char *prettify_refname (const char *name) { -return name + ( -starts_with (name,"refs/heads/" ) ?11 : -starts_with (name,"refs/tags/" ) ?10 : -starts_with (name,"refs/remotes/" ) ?13 : -0 ); +if (skip_prefix (name,"refs/heads/" , &name) || +skip_prefix (name,"refs/tags/" , &name) || +skip_prefix (name,"refs/remotes/" , &name)) + ;/* nothing */ +return name;Nice, but why add the "if" when it's doing nothing?
Quoted 1 line
It's short-circuiting in the conditional.
I think René meant this:
/* just for side effects */
skip_prefix(name, "refs/heads/", &name) ||
skip_prefix(name, "refs/tags/", &name) ||
skip_prefix(name, "refs/remotes/", &name);
return name;which still short-sircuits, even though I do think it looks strange; "correct but strange".
Am 23.03.2017 um 20:33 schrieb Junio C Hamano:
Quoted 44 lines
Quoted 31 lines · Jeff King
Quoted 14 lines · René Scharfe
Am 23.03.2017 um 16:50 schrieb SZEDER Gábor:
Quoted 13 lines
This eliminates three magic numbers.
Signed-off-by SZEDER Gábor <[email protected]>--- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)refs.c +0
-0@@ -366,11 +366,11 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)Quoted 14 lines
refs.c int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data) · line 373
Quoted 12 lines
refs.c +5
-5const char *prettify_refname (const char *name) { -return name + ( -starts_with (name,"refs/heads/" ) ?11 : -starts_with (name,"refs/tags/" ) ?10 : -starts_with (name,"refs/remotes/" ) ?13 : -0 ); +if (skip_prefix (name,"refs/heads/" , &name) || +skip_prefix (name,"refs/tags/" , &name) || +skip_prefix (name,"refs/remotes/" , &name)) + ;/* nothing */ +return name;Nice, but why add the "if" when it's doing nothing?
Quoted 1 line
It's short-circuiting in the conditional.
I think René meant this:
/* just for side effects */ skip_prefix(name, "refs/heads/", &name) || skip_prefix(name, "refs/tags/", &name) || skip_prefix(name, "refs/remotes/", &name); return name;
Quoted 2 lines
which still short-sircuits, even though I do think it looks strange; "correct but strange".
Yes. At least to me it looks less strange than the same lines wrapped in "if ... /* nothing */".
René
Quoted 13 lines · Junio C Hamano
Quoted 3 lines
Quoted 1 line
Nice, but why add the "if" when it's doing nothing?
It's short-circuiting in the conditional.
I think René meant this:
/* just for side effects */ skip_prefix(name, "refs/heads/", &name) || skip_prefix(name, "refs/tags/", &name) || skip_prefix(name, "refs/remotes/", &name); return name;
Quoted 2 lines
which still short-sircuits, even though I do think it looks strange; "correct but strange".
And it causes the compiler to complain that the value is not used.
-Peff
Quoted 7 lines · René Scharfe
Quoted 7 lines
I think René meant this:
/* just for side effects */ skip_prefix(name, "refs/heads/", &name) || skip_prefix(name, "refs/tags/", &name) || skip_prefix(name, "refs/remotes/", &name);
Quoted 7 lines
Quoted 4 lines
return name;which still short-sircuits, even though I do think it looks strange; "correct but strange".
Yes. At least to me it looks less strange than the same lines wrapped in "if ... /* nothing */".
Yup, after looking at it again, it does not look so "strange" to me. I probably should have said "unusual but correct", not "correct but strange".
Quoted 13 lines · Jeff King
Quoted 11 lines · Junio C Hamano
Quoted 3 lines
Quoted 1 line
Nice, but why add the "if" when it's doing nothing?
It's short-circuiting in the conditional.
I think René meant this:
/* just for side effects */ skip_prefix(name, "refs/heads/", &name) || skip_prefix(name, "refs/tags/", &name) || skip_prefix(name, "refs/remotes/", &name);
Quoted 6 lines
Quoted 4 lines
return name;which still short-sircuits, even though I do think it looks strange; "correct but strange".
And it causes the compiler to complain that the value is not used.
Ahh. OK.
In any case, I've queued the original with "if", which shouldn't have that problem ;-)
Quoted 6 lines · Jeff King
Quoted 4 lines · Junio C Hamano
Quoted 3 lines
Quoted 1 line
Nice, but why add the "if" when it's doing nothing?
It's short-circuiting in the conditional.
Quoted 8 lines
Quoted 8 lines
I think René meant this:
/* just for side effects */ skip_prefix(name, "refs/heads/", &name) || skip_prefix(name, "refs/tags/", &name) || skip_prefix(name, "refs/remotes/", &name); return name;
That was my first version indeed. Unfortunately:
Quoted 1 line
And it causes the compiler to complain that the value is not used.
Exactly. So that 'if' was a way to shut up the compiler complaining about the unused result of the ||-chain. Perhaps not the best way to do that, but I couldn't come up with any better.
I had a note about it attached to the commit, but then run 'format-patch' without '--notes'... oh, well.
I guess I should have wrote that in the commit message to begin with, shouldn't I? Or in an in-code comment.
Am 23.03.2017 um 20:39 schrieb Jeff King:
Quoted 13 lines · Junio C Hamano
Quoted 12 lines
Quoted 4 lines
Quoted 2 lines
Nice, but why add the "if" when it's doing nothing?
It's short-circuiting in the conditional.
I think René meant this:
/* just for side effects */ skip_prefix(name, "refs/heads/", &name) || skip_prefix(name, "refs/tags/", &name) || skip_prefix(name, "refs/remotes/", &name);
Quoted 6 lines
Quoted 4 lines
return name;which still short-sircuits, even though I do think it looks strange; "correct but strange".
And it causes the compiler to complain that the value is not used.
Ah, that explains it, thanks. The "if" is strange, but wrapping the expression in (void)(...) isn't better.
Clang doesn't warn about the code above by the way.
René