[PATCH] ui-refs.c: Simplify and inline cmp_age()

[PATCH] ui-refs.c: Simplify and inline cmp_age()

[cgit]

In comparison functions, only the sign of the return value matters -- no
need to have extra checks to ensure the return value always is +/-1 when
the actual parameters differ. After removing all the checks, the
function body boils down to

    return age2 - age1;

so we can as well replace all invocations with that subtraction.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-refs.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/ui-refs.c b/ui-refs.c
index 147b665..8a4ce74 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -11,20 +11,6 @@
 #include "html.h"
 #include "ui-shared.h"
 
-static int cmp_age(int age1, int age2)
-{
-	if (age1 != 0 && age2 != 0)
-		return age2 - age1;
-
-	if (age1 == 0 && age2 == 0)
-		return 0;
-
-	if (age1 == 0)
-		return +1;
-
-	return -1;
-}
-
 static int cmp_ref_name(const void *a, const void *b)
 {
 	struct refinfo *r1 = *(struct refinfo **)a;
@@ -38,7 +24,7 @@ static int cmp_branch_age(const void *a, const void *b)
 	struct refinfo *r1 = *(struct refinfo **)a;
 	struct refinfo *r2 = *(struct refinfo **)b;
 
-	return cmp_age(r1->commit->committer_date, r2->commit->committer_date);
+	return r2->commit->committer_date - r1->commit->committer_date;
 }
 
 static int get_ref_age(struct refinfo *ref)
@@ -59,7 +45,7 @@ static int cmp_tag_age(const void *a, const void *b)
 	struct refinfo *r1 = *(struct refinfo **)a;
 	struct refinfo *r2 = *(struct refinfo **)b;
 
-	return cmp_age(get_ref_age(r1), get_ref_age(r2));
+	return get_ref_age(r2)- get_ref_age(r1);
 }
 
 static int print_branch(struct refinfo *ref)
-- 
1.8.5.3

[PATCH] ui-refs.c: Simplify and inline cmp_age()

[Jason]

On Tue, Feb 4, 2014 at 8:22 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
>
>     return age2 - age1;
> -static int cmp_age(int age1, int age2)
> -{
> -       if (age1 != 0 && age2 != 0)
> -               return age2 - age1;
> -
> -       if (age1 == 0 && age2 == 0)
> -               return 0;
> -
> -       if (age1 == 0)
> -               return +1;
> -
> -       return -1;
> -}


cmp_age(-1, 0) returns -1

0 - -1 returns 1

[PATCH] ui-refs.c: Simplify and inline cmp_age()

[Jason]

zx2c4 at thinkpad ~ $ cat a.c
static int cmp_age(int age1, int age2)
{
       if (age1 != 0 && age2 != 0)
               return age2 - age1;

       if (age1 == 0 && age2 == 0)
               return 0;

       if (age1 == 0)
               return +1;

       return -1;
}

static int cmp_age2(int age1, int age2)
{
        return age2 - age1;
}

void main()
{
        printf("%d %d\n", cmp_age(-1, 0), cmp_age2(-1, 0));
}
zx2c4 at thinkpad ~ $ gcc a.c
zx2c4 at thinkpad ~ $ ./a.out
-1 1

[PATCH] ui-refs.c: Simplify and inline cmp_age()

[cgit]

On Tue, 04 Feb 2014 at 20:26:32, Jason A. Donenfeld wrote:
> On Tue, Feb 4, 2014 at 8:22 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> >
> >     return age2 - age1;
> > -static int cmp_age(int age1, int age2)
> > -{
> > -       if (age1 != 0 && age2 != 0)
> > -               return age2 - age1;
> > -
> > -       if (age1 == 0 && age2 == 0)
> > -               return 0;
> > -
> > -       if (age1 == 0)
> > -               return +1;
> > -
> > -       return -1;
> > -}
> 
> 
> cmp_age(-1, 0) returns -1
> 
> 0 - -1 returns 1

Can the age ever be negative?

[PATCH] ui-refs.c: Simplify and inline cmp_age()

[cgit]

On Tue, 04 Feb 2014 at 20:27:56, Lukas Fleischer wrote:
> On Tue, 04 Feb 2014 at 20:26:32, Jason A. Donenfeld wrote:
> [...]
> > cmp_age(-1, 0) returns -1
> > 
> > 0 - -1 returns 1
> 
> Can the age ever be negative?

The age parameters can only contain copies of the committer_date or
tagger_date fields which are unsigned. So I guess the patch is fine.

[PATCH] ui-refs.c: Simplify and inline cmp_age()

[Jason]

I haven't had any time to work through all the logic for why this
might be the case. Are any volunteers who can triple check that the
change in logic here is not an issue? I'd like to be extra certain
before merging this.

On Tue, Feb 4, 2014 at 9:07 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
>
> On Tue, 04 Feb 2014 at 20:22:05, Lukas Fleischer wrote:
> > In comparison functions, only the sign of the return value matters -- no
> > need to have extra checks to ensure the return value always is +/-1 when
> > the actual parameters differ. After removing all the checks, the
> > function body boils down to
> >
> >     return age2 - age1;
> >
> > so we can as well replace all invocations with that subtraction.
> >
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> >  ui-refs.c | 18 ++----------------
> >  1 file changed, 2 insertions(+), 16 deletions(-)
> >
>
> Lars, we're not quite sure whether this patch is correct. I can't see
> where the version not having the extra checks behaves different from the
> original function but we might be missing something. Do you remember why
> you implemented cmp_age() like this in the first place?
>
> Please Cc Jason when replying. Thanks!
>
> > diff --git a/ui-refs.c b/ui-refs.c
> > index 147b665..8a4ce74 100644
> > --- a/ui-refs.c
> > +++ b/ui-refs.c
> > @@ -11,20 +11,6 @@
> >  #include "html.h"
> >  #include "ui-shared.h"
> >
> > -static int cmp_age(int age1, int age2)
> > -{
> > -       if (age1 != 0 && age2 != 0)
> > -               return age2 - age1;
> > -
> > -       if (age1 == 0 && age2 == 0)
> > -               return 0;
> > -
> > -       if (age1 == 0)
> > -               return +1;
> > -
> > -       return -1;
> > -}
> > -
> >  static int cmp_ref_name(const void *a, const void *b)
> >  {
> >         struct refinfo *r1 = *(struct refinfo **)a;
> > @@ -38,7 +24,7 @@ static int cmp_branch_age(const void *a, const void *b)
> >         struct refinfo *r1 = *(struct refinfo **)a;
> >         struct refinfo *r2 = *(struct refinfo **)b;
> >
> > -       return cmp_age(r1->commit->committer_date, r2->commit->committer_date);
> > +       return r2->commit->committer_date - r1->commit->committer_date;
> >  }
> >
> >  static int get_ref_age(struct refinfo *ref)
> > @@ -59,7 +45,7 @@ static int cmp_tag_age(const void *a, const void *b)
> >         struct refinfo *r1 = *(struct refinfo **)a;
> >         struct refinfo *r2 = *(struct refinfo **)b;
> >
> > -       return cmp_age(get_ref_age(r1), get_ref_age(r2));
> > +       return get_ref_age(r2)- get_ref_age(r1);
> >  }
> >
> >  static int print_branch(struct refinfo *ref)
> > --
> > 1.8.5.3
> >
> > _______________________________________________
> > CGit mailing list
> > CGit at lists.zx2c4.com
> > http://lists.zx2c4.com/mailman/listinfo/cgit