Re: [PATCH v2] Rename suffixcmp() to has_suffix() and inverse its result

From: Max Horn <max@quendi.de>
Date: Nov 5, 2013, 18:39 UTC
Message-ID: <C9A6A1E9-E08C-4C9B-B18D-CED751ACB975@quendi.de>
In-Reply-To: <20131105045722.20276.20853.chriscool@tuxfamily.org>

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

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

Show 9 quoted lines

> 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

Show 6 quoted lines

> 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"
but
  suffixcmp("3", "2") = 1    so "3" > "2"

Show 12 quoted lines

> 
> Signed-off-by: Christian Couder <chriscool@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().
>

[...]