-
-
Notifications
You must be signed in to change notification settings - Fork 134
Swap definitions of isWhitespace and isSpace #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Swap definitions of isWhitespace and isSpace #27
Conversation
Previously, the behavior of isWhitespace did not match what was stated in the documentation: Analyse if a char is a white space, that is space, formfeed ('\f'), newline ('\n'), carriage return ('\r'), horizontal tab ('\t'), and vertical tab ('\v')). Returns true if thisChar contains a white space. Previously, the behavior of isSpace did not match what was stated in the documentation: Analyse if a char is the space character. Returns true if thisChar contains the space character. A strict reading of the documentation for isSpace would indicate that it should only match on ASCII value 32 (true space). The change made here will cause it to match only a true space or a tab.
Thanks for taking time to tackle this! I'd prefer to change the documentation (the actual behavior "makes sense" for me). But we should reach a bigger consensus before merging.
@tigoe @sandeepmistry @cmaglie @mbanzi any thoughts?
Documentation for isspace
definitely indicates that it's intended to check for white-space characters:
- http://www.cplusplus.com/reference/cctype/isspace/
- https://en.cppreference.com/w/cpp/string/byte/isspace
I didn't find anything more official to back that. Here's a quote from the closest draft to the C++11 standard, section 2.5, paragraph 2:
or white-space characters (space, horizontal tab, new-line, vertical tab, and form-feed)
That's not a formal definition of white-space, but it does make it seem that a function named isWhitespace
would ideally match on more than only space and tab.
That said, I am favorable to the argument that long-established core functions should not suddenly change their behavior, even if that behavior doesn't match the function name so well. I'm just not convinced by the argument that the current behavior "makes sense".
pnndra
commented
May 7, 2019
although i understand that common naming would suggest that the code fix is the right thing on the other hand it may potentially break existing applications so after a quick consultation within Arduino we prefer to fix documentation rather than change code that could break existing applications.
Good call. I will make the documentation change.
Uh oh!
There was an error while loading. Please reload this page.
Previously, the behavior of
isWhitespace
did not match what was stated in the documentation:Previously, the behavior of
isSpace
did not match what was stated in the documentation:A strict reading of the documentation for
isSpace
would indicate that it should only match on ASCII value 32 (true space). The change made here will cause it to match only a true space or a tab.I realize this is a breaking change. It is the action recommended in the issue report:
arduino/Arduino#7041 (comment)
The alternative is to leave the code as-is and correct the documentation, which I am happy to do if the decision goes that way.
There is also the question of how
isSpace
should work. I can make it match on space, and not on tab if that's preferable. If left as it currently is in this PR, the documentation will need to be updated (once the updated code has been released) to indicate that it also matches on tab.I'm submitting the PR to this repo as it seems that ArduinoCore-API should now act as the model for the non-architecture specific code in other cores until the time comes for them to transition to using ArduinoCore-API directly. If accepted here, I can submit the equivalent pull requests to the other core repositories.
Fixes arduino/Arduino#7041
Reference: http://forum.arduino.cc/index.php?topic=597290
Demonstration sketch: