Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 21, 2014, 02:48 UTC
Message-ID: <CAPig+cTw8pyRVOHToGRPBdxv+TX8Vcj5OrX-CmLWRCigZRS4MA@mail.gmail.com>
In-Reply-To: <1395344384-7975-1-git-send-email-g3orge.app@gmail.com>

On Thu, Mar 20, 2014 at 3:39 PM, George Papanikolaou <g3orge.app@gmail.com> wrote:

> Removing the bloat of checking for both '\r' and '\n' with the prettier
> iswspace() function which checks for other characters as well. (read: \f \t \v)

Use imperative mood. "Remove" rather than "Removing".

Bloat? Prettier? Subjective stuff.

Did you verify that it is safe to strip all whitespace characters rather than only line-endings? Perhaps say so in the commit message.

Why the choice of iswspace()? These are normal-width character strings, so why apply a wide-character function?

More below.

Show 27 quoted lines

> ---
>
> This is one more try to clean up this fuzzy_matchlines() function as part of a
> microproject for GSOC. The rest more clarrified microprojects were taken.
> I'm obviously planning on applying.
>
> Thanks
>
> Signed-of-by: George 'papanikge' Papanikolaou <g3orge.app@gmail.com>
>
>  builtin/apply.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index b0d0986..912a53a 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -295,9 +295,9 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
>         int result = 0;
>
>         /* ignore line endings */
> -       while ((*last1 == '\r') || (*last1 == '\n'))
> +       while (iswspace(*last1))
>                 last1--;
> -       while ((*last2 == '\r') || (*last2 == '\n'))
> +       while (iswspace(*last2))
>                 last2--;

Doesn't this change turn the comment preceding this code into a half-truth? Perhaps update the comment?

>         /* skip leading whitespace */
> --
> 1.9.0