Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
per1234 wants to merge 1 commit into arduino:master from per1234:swap-iswhitespace-isspace-definitions
Closed

Swap definitions of isWhitespace and isSpace #27

per1234 wants to merge 1 commit into arduino:master from per1234:swap-iswhitespace-isspace-definitions

Conversation

Copy link
Collaborator

@per1234 per1234 commented Feb 12, 2019
edited
Loading

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.


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:

void setup() {
 Serial.begin(9600);
 while (!Serial) {}
 const char chars[] = {'\f', '\n', '\r', '\v', '\t', ' ', 'a'};
 for (byte charCounter = 0; charCounter < sizeof(chars); charCounter++) {
 Serial.print("isWhitespace(");
 Serial.print((byte)chars[charCounter]);
 Serial.print(") == ");
 Serial.println(isWhitespace(chars[charCounter]) ? "true" : "false");
 Serial.print("isSpace(");
 Serial.print((byte)chars[charCounter]);
 Serial.print(") == ");
 Serial.println(isSpace(chars[charCounter]) ? "true" : "false");
 }
}
void loop() {}

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.
Copy link
Member

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?

Copy link
Member

tigoe commented Feb 13, 2019 via email

Seems to make sense to change to me. The original idea should have been as the documentation says. T
...
On Wed, Feb 13, 2019, 3:27 AM Martino Facchin ***@***.***> wrote: 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 <https://github.com/tigoe> @sandeepmistry <https://github.com/sandeepmistry> @cmaglie <https://github.com/cmaglie> @mbanzi <https://github.com/mbanzi> any thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXOn2I8wzJwAGIuDujEMIefbqLsQJ7bks5vM8yBgaJpZM4a4SGZ> .

Copy link
Collaborator Author

per1234 commented Feb 13, 2019

Documentation for isspace definitely indicates that it's intended to check for white-space characters:

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

Copy link

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.

Copy link
Collaborator Author

per1234 commented May 7, 2019

Good call. I will make the documentation change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] isWhitespace() / isSpace()

AltStyle によって変換されたページ (->オリジナル) /