-
-
Notifications
You must be signed in to change notification settings - Fork 140
Made WCharacter.h more clear #26
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
Conversation
a == 0 ? false : true is same as a != 0 Also added extern C
CLA assistant check
All committers have signed the CLA.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@matthijskooijman
matthijskooijman
left a comment
There was a problem hiding this 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.
There was a problem hiding this comment.
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
commented
May 25, 2021
Hi @leg0 👋 Can you please sign the CLA?
leg0
commented
May 25, 2021
@aentinger I have already signed it.
per1234
commented
May 25, 2021
We need @pratikpc to sign the CLA:
https://cla-assistant.io/arduino/ArduinoCore-API?pullRequest=26
aentinger
commented
May 25, 2021
@aentinger I have already signed it.
Mea culpa 😊
aentinger
commented
Jun 10, 2021
@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.
It's obvious that
a == 0 ? false : trueis same asa != 0Also 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 : trueinstead of the more easy to reada != 0which strikes a fine balance between simplicity and readability.If we are to look at Standard Library Definitions of the various functions used like isdigit,
isalphaetc. we find they return 0 when they do not find anything thus makingisdigit(ch) != 0even more clearerAdded 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