Skip to content
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

Made WCharacter.h more clear #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Made WCharacter.h more clear #26

wants to merge 1 commit into from

Conversation

pratikpc
Copy link

@pratikpc pratikpc commented Feb 6, 2019

It's obvious that a == 0 ? false : true is same as a != 0

Also a != 0 looks more clear as a Return Type to a bool function.
It's more easier to understand what it does in this form.
WCharacter.h was full of functions which utilised a == 0 ? false : true instead of the more easy to read a != 0 which strikes a fine balance between simplicity and readability.

If we are to look at Standard Library Definitions of the various functions used like isdigit, isalpha etc. we find they return 0 when they do not find anything thus making isdigit(ch) != 0 even more clearer

Added extern C
Given the fact that we are using C Functions and code that is compatible with the C Standard, I added extern C. Extern C is also used in similar C Standard Compatible Arduino header files

a == 0 ? false : true is same as a != 0
Also added extern C
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@@ -22,6 +22,12 @@

#include <ctype.h>

// This Mentions that the following code is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useless. It only changes how the function names are mangled. Since the functions are not compiled into an object file nor a static library, it doesn't matter if they're extern "C" or not. They're inline, in C++ and in C.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true for any inline function, since they are not necessarily inlined. In this case, they are always_inline so it should indeed not matter, but it might be good to still have the extern "C" for good measure (and in case other non-always_inline functions are added later). I would omit the comment here, though, since the meaning/intention of extern "C" is rather obvious (and can be looked up if not) and I believe it's used without comment elsewhere too.

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a reasonable change to me. As @leg0 suggests, extern "C" is not strictly required, but it doesn't hurt. I can imagine removing the comment about it, though. See also inline comment.

@@ -22,6 +22,12 @@

#include <ctype.h>

// This Mentions that the following code is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true for any inline function, since they are not necessarily inlined. In this case, they are always_inline so it should indeed not matter, but it might be good to still have the extern "C" for good measure (and in case other non-always_inline functions are added later). I would omit the comment here, though, since the meaning/intention of extern "C" is rather obvious (and can be looked up if not) and I believe it's used without comment elsewhere too.

@aentinger
Copy link
Contributor

Hi @leg0 👋 Can you please sign the CLA?

@leg0
Copy link
Contributor

leg0 commented May 25, 2021

@aentinger I have already signed it.

@per1234
Copy link
Collaborator

per1234 commented May 25, 2021

We need @pratikpc to sign the CLA:
https://cla-assistant.io/arduino/ArduinoCore-API?pullRequest=26

@aentinger
Copy link
Contributor

@aentinger I have already signed it.

Mea culpa 😊

@aentinger
Copy link
Contributor

@pratikpc can you please sign it while being logged in via the right email ... likely the email stored in the commit is different from the one you are signed in GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants