* [PATCH] refs.c: use skip_prefix() in prettify_refname() @ 2017-03-23 15:50 SZEDER Gábor 2017-03-23 19:18 ` René Scharfe 0 siblings, 1 reply; 11+ messages in thread From: SZEDER Gábor @ 2017-03-23 15:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, SZEDER Gábor This eliminates three magic numbers. Signed-off-by: SZEDER Gábor <szeder.dev@gmail•com> --- refs.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/refs.c b/refs.c index e7606716d..0272e332c 100644 --- a/refs.c +++ b/refs.c @@ -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[] = { -- 2.12.1.485.g1616aa492 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 15:50 [PATCH] refs.c: use skip_prefix() in prettify_refname() SZEDER Gábor @ 2017-03-23 19:18 ` René Scharfe 2017-03-23 19:23 ` Jeff King 0 siblings, 1 reply; 11+ messages in thread From: René Scharfe @ 2017-03-23 19:18 UTC (permalink / raw) To: SZEDER Gábor, Junio C Hamano; +Cc: git Am 23.03.2017 um 16:50 schrieb SZEDER Gábor: > This eliminates three magic numbers. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail•com> > --- > refs.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/refs.c b/refs.c > index e7606716d..0272e332c 100644 > --- a/refs.c > +++ b/refs.c > @@ -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; Nice, but why add the "if" when it's doing nothing? René ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:18 ` René Scharfe @ 2017-03-23 19:23 ` Jeff King 2017-03-23 19:31 ` Stefan Beller 2017-03-23 19:33 ` Junio C Hamano 0 siblings, 2 replies; 11+ messages in thread From: Jeff King @ 2017-03-23 19:23 UTC (permalink / raw) To: René Scharfe; +Cc: SZEDER Gábor, Junio C Hamano, git On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote: > Am 23.03.2017 um 16:50 schrieb SZEDER Gábor: > > This eliminates three magic numbers. > > > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail•com> > > --- > > refs.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/refs.c b/refs.c > > index e7606716d..0272e332c 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -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; > > Nice, but why add the "if" when it's doing nothing? It's short-circuiting in the conditional. -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:23 ` Jeff King @ 2017-03-23 19:31 ` Stefan Beller 2017-03-23 19:33 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Stefan Beller @ 2017-03-23 19:31 UTC (permalink / raw) To: Jeff King Cc: René Scharfe, SZEDER Gábor, Junio C Hamano, git@vger•kernel.org On Thu, Mar 23, 2017 at 12:23 PM, Jeff King <peff@peff•net> wrote: > On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote: > >> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor: >> > This eliminates three magic numbers. >> > >> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail•com> >> > --- >> > refs.c | 10 +++++----- >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> > >> > diff --git a/refs.c b/refs.c >> > index e7606716d..0272e332c 100644 >> > --- a/refs.c >> > +++ b/refs.c >> > @@ -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; >> >> Nice, but why add the "if" when it's doing nothing? > > 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 > > -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:23 ` Jeff King 2017-03-23 19:31 ` Stefan Beller @ 2017-03-23 19:33 ` Junio C Hamano 2017-03-23 19:36 ` René Scharfe 2017-03-23 19:39 ` Jeff King 1 sibling, 2 replies; 11+ messages in thread From: Junio C Hamano @ 2017-03-23 19:33 UTC (permalink / raw) To: Jeff King; +Cc: René Scharfe, SZEDER Gábor, git Jeff King <peff@peff•net> writes: > On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote: > >> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor: >> > This eliminates three magic numbers. >> > >> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail•com> >> > --- >> > refs.c | 10 +++++----- >> > 1 file changed, 5 insertions(+), 5 deletions(-) >> > >> > diff --git a/refs.c b/refs.c >> > index e7606716d..0272e332c 100644 >> > --- a/refs.c >> > +++ b/refs.c >> > @@ -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; >> >> 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; which still short-sircuits, even though I do think it looks strange; "correct but strange". ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:33 ` Junio C Hamano @ 2017-03-23 19:36 ` René Scharfe 2017-03-23 19:40 ` Junio C Hamano 2017-03-23 19:39 ` Jeff King 1 sibling, 1 reply; 11+ messages in thread From: René Scharfe @ 2017-03-23 19:36 UTC (permalink / raw) To: Junio C Hamano, Jeff King; +Cc: SZEDER Gábor, git Am 23.03.2017 um 20:33 schrieb Junio C Hamano: > Jeff King <peff@peff•net> writes: > >> On Thu, Mar 23, 2017 at 08:18:26PM +0100, René Scharfe wrote: >> >>> Am 23.03.2017 um 16:50 schrieb SZEDER Gábor: >>>> This eliminates three magic numbers. >>>> >>>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail•com> >>>> --- >>>> refs.c | 10 +++++----- >>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/refs.c b/refs.c >>>> index e7606716d..0272e332c 100644 >>>> --- a/refs.c >>>> +++ b/refs.c >>>> @@ -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; >>> >>> 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; > > 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é ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:36 ` René Scharfe @ 2017-03-23 19:40 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2017-03-23 19:40 UTC (permalink / raw) To: René Scharfe; +Cc: Jeff King, SZEDER Gábor, git René Scharfe <l.s.r@web•de> writes: >> 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". > > 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". ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:33 ` Junio C Hamano 2017-03-23 19:36 ` René Scharfe @ 2017-03-23 19:39 ` Jeff King 2017-03-23 20:05 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Jeff King @ 2017-03-23 19:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: René Scharfe, SZEDER Gábor, git On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote: > >> 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; > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:39 ` Jeff King @ 2017-03-23 20:05 ` Junio C Hamano 2017-03-23 20:06 ` SZEDER Gábor 2017-03-23 20:06 ` René Scharfe 2 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2017-03-23 20:05 UTC (permalink / raw) To: Jeff King; +Cc: René Scharfe, SZEDER Gábor, git Jeff King <peff@peff•net> writes: > On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote: > >> >> 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; >> >> 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 ;-) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:39 ` Jeff King 2017-03-23 20:05 ` Junio C Hamano @ 2017-03-23 20:06 ` SZEDER Gábor 2017-03-23 20:06 ` René Scharfe 2 siblings, 0 replies; 11+ messages in thread From: SZEDER Gábor @ 2017-03-23 20:06 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Git mailing list On Thu, Mar 23, 2017 at 8:39 PM, Jeff King <peff@peff•net> wrote: > On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote: > >> >> 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; That was my first version indeed. Unfortunately: > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] refs.c: use skip_prefix() in prettify_refname() 2017-03-23 19:39 ` Jeff King 2017-03-23 20:05 ` Junio C Hamano 2017-03-23 20:06 ` SZEDER Gábor @ 2017-03-23 20:06 ` René Scharfe 2 siblings, 0 replies; 11+ messages in thread From: René Scharfe @ 2017-03-23 20:06 UTC (permalink / raw) To: Jeff King, Junio C Hamano; +Cc: SZEDER Gábor, git Am 23.03.2017 um 20:39 schrieb Jeff King: > On Thu, Mar 23, 2017 at 12:33:06PM -0700, Junio C Hamano wrote: > >>>> 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; >> >> 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é ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-23 20:06 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-03-23 15:50 [PATCH] refs.c: use skip_prefix() in prettify_refname() SZEDER Gábor 2017-03-23 19:18 ` René Scharfe 2017-03-23 19:23 ` Jeff King 2017-03-23 19:31 ` Stefan Beller 2017-03-23 19:33 ` Junio C Hamano 2017-03-23 19:36 ` René Scharfe 2017-03-23 19:40 ` Junio C Hamano 2017-03-23 19:39 ` Jeff King 2017-03-23 20:05 ` Junio C Hamano 2017-03-23 20:06 ` SZEDER Gábor 2017-03-23 20:06 ` René Scharfe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox