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

[Eric Sunshine <sunshine@sunshineco.com>]

[Please reply on-list to review comments. Other people may learn from
the discussion or have comments of their own.]

On Fri, Mar 21, 2014 at 6:00 PM, George Papanikolaou
<g3orge.app@gmail.com> wrote:
> On Fri, Mar 21, 2014 at 4:48 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> 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?
>>
> why not?

Because it's unnecessary and invites confusion from people reading
code since they now have to wonder if there is something unusual and
non-obvious going on. Worse, the two loops immediately below the ones
you changed, as well as the rest of the function, use plain isspace(),
which really ramps up the "huh?"-factor from the reader.

The original code has the asset of being clear and obvious. Changing
these two loops to use a wide-character function makes it less so.

> since at this point it is checking for any non-readable
> characters at the end of the buffer, I figured we should check for the
> "wide-character" function that covers these.

Neither the function comment nor the existing code implies that it is
checking for "any non-readable characters". (I'm not even sure what
that means.) The only thing the existing code says at that point is
that it is ignoring line-endings.

> It is true that the
> comment should change in that matter.
>
> Also why wouldn't it be safe? And how can I check?

You're changing the behavior of the function (assuming I'm reading
correctly), which is why I asked if you verified that doing so was
safe. The existing code considers "foo bar" and "foo bar " to be
different. With your change, they are considered equal, which is
actually more in line with what the function comment says.
Nevertheless, callers may be relying upon the existing behavior.

At the very least, the unit tests should be run as a quick check of
whether this behavior change introduces problems. Manual inspection of
callers also wouldn't hurt.

There's also the issue that Michael raised when he asked what would
happen if either string was composed of whitespace only. The existing
code is not robust and can crash, but your change may increase the
likelihood of the crash.

> Thanks
>
> --
> papanikge's surrogate email.
> I may reply back.
> http://www.5slingshots.com/