Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 21, 2014, 23:07 UTC
- Message-ID
- <CAPig+cRcW8jv7LNZmLfrSGLaqE7yHycbmfvtNETPo51QoM7N2g@mail.gmail.com>
- In-Reply-To
- <CAByyCQBmCTfW0HBL04MMqwm+bDe4Rb6n+MfWdYUQ6M6yW_u=yw@mail.gmail.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:
Show 9 quoted lines
> 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.
Show 6 quoted lines
> Thanks > > -- > papanikge's surrogate email. > I may reply back. > http://www.5slingshots.com/