On Sat, Mar 22, 2014 at 5:33 AM, George Papanikolaou <g3orge.app@xxxxxxxxx> wrote: > 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 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. -- 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