On Sat, Mar 22, 2014 at 12:46 AM, Eric Sunshine <sunsh...@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

> 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.

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

Thanks again for the feedback.

papanikge's surrogate email.
I may reply back.
http://www.5slingshots.com/I did not think about that possibility.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to