+1 for the change. I find the resulting code easier to understand, too.

On 05.11.2013, at 05:57, Christian Couder <chrisc...@tuxfamily.org> wrote:

> As suffixcmp() should not be used as an ordering comparison function,
> and anything-cmp() ought to be usable as an ordering comparison function,
> suffixcmp() should be renamed to something that doesn't end with "cmp".
> has_suffix() is a straightforward name for such a function, except
> that with such a name callers will expect that it will return 1
> when the suffix is present and 0 otherwise.
> So we need to also inverse the value returned by this function ti

> match what the callers will expect, because suffixcmp() like all
> anything-cmp() returns 0 when the suffix is present and 1 or -1
> otherwise.
> As we inverse the value returned by the function, we also have
> to inverse the ways its callers are using its returned value.

s/inverse/invert/  (multiple times)

Taking one step back, shouldn't the commit message rather explain the new 
status, instead of referring so much to the past? If I imagine somebody reading 
this in a year, they might not even know suffixcmp (e.g. if they joined the 
project after this patch was merged).

How about something like this:

--- 8< ----

Rename suffixcmp() to has_suffix() and invert its result

Now has_suffix() returns 1 when the suffix is present and 0 otherwise.

The old name followed the pattern anything-cmp(), which suggests
a general comparison function suitable for e.g. sorting objects.
But this was not the case for suffixcmp().

--- 8< ----

By the way, a much stronger reason why suffixcmp is not suitable than that it 
is not clear what it would mean, is that it is not transitive. I.e. for an 
ordering you would want that if a<b and b<c then a<c. This is /was not the case 
for suffixcmp:

  suffixcmp("3", "31") = -1  (because "31" is longer than "3"), so "3" < "31"
  suffixcmp("31", "2") = -1  (because "1" < "2"), so "31" < "2"
  suffixcmp("3", "2") = 1    so "3" > "2"

> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
> ---
> Hi Junio and Peff,
> So here is a new version of the patch to rename
> suffixcmp() into has_suffix(). We now inverse the
> result of the function as we rename it.
> This patch should be added to or squashed into the
> patch series that removes postfixcmp().


Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail

Reply via email to