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

From: Eric Sunshine <sunshine@sunshineco.com>
To: George Papanikolaou <g3orge.app@gmail.com>
Cc: Git List <git@vger.kernel.org>, Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars
Date: Sun, 23 Mar 2014 05:35:43 -0400	[thread overview]
Message-ID: <CAPig+cTFNsmQPmUpax-rbqkk5JzgAw4fK0tM4U013Z_x7o-ZyA@mail.gmail.com> (raw)
In-Reply-To: <CAByyCQAqZnnc91ZgmxdKgc7T0POLqd+iXmKvaKEPMOx6CNQkKQ@mail.gmail.com>

On Sat, Mar 22, 2014 at 5:33 AM, George Papanikolaou
<g3orge.app@gmail.com> wrote:
> On Sat, Mar 22, 2014 at 12:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Because it's unnecessary and invites confusion from people reading the
>> code since they now have to wonder if there is something unusual and
>> non-obvious going. 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.
>>
> Yes I understand it does add a factor of ambiguity.
>
>> 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.
>>
> I mean characters that are not printable like letters, numbers, dots etc

It's still not clear how this answer relates to my question about why
you used iswspace() rather than isspace().

Nothing in the code or comments indicates that it wants to ignore
non-printing characters. Even if the intention of your change had
indeed been to ignore such characters, you would have used !isprint()
or !iswprint().

>> You're changing the behavior of the function (assuming I'm reading it
>> 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 if this behavior change introduces problems. Manual inspection
>> of callers also wouldn't hurt.
>>
> I did not think about that possibility, because I ran `make` and the
> tests passed so I thought that that would be ok.

Unit tests may cover a lot of functionality, but there will always be
holes in the coverage. Thus, it's a good idea to examine callers and
surrounding code manually, as well.

Since this is a behavior change, it deserves mention in the commit
message, as well as assurance that you verified (as best you can) that
it did not break existing callers. (It also wouldn't hurt to mention
that it brings the code more in line with the function documentation.)

> Anyway, do you have any ideas on how to improve that function?

Michael gave you a strong clue when he asked what would happen, with
your change in place, if the string consisted only of whitespace. The
loops you touched are already fragile, even without your change.
Making them more robust would likely be considered an improvement.

> Thanks again for the feedback.
>
> --
> papanikge's surrogate email.
> I may reply back.
> http://www.5slingshots.com/I did not think about that possibility.