On Sat, Mar 22, 2014 at 12:46 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html