- Notifications
You must be signed in to change notification settings - Fork13.3k
WString: return bool instead of unsigned char#7939
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Clean up the intent, resulting assembly stays the same.
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.
Thanks, I really like the idea,@mcspr !
That said, I'm worried about changing the signature of something as basic asString. Even today, in the ArduinoAPI repoString.h usesunsigned char. So this change would move our interface away from that, and break anything inheriting from String.
https://github.com/earlephilhower/ArduinoCore-API/blob/9da53735171f7cccbe781c66d9ee1068d94d6ec8/api/String.h
So I'm honestly not sure which way to go here. Let's discuss and see if we can get anyone else's input.
I may be missing the actual issue, but since this is not virtual inheritance methods do not depend on the base signature: Also opened a PR to the ArduinoCore-API with a similar change |
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.
Solid explanation. I'm happy to give this a shot and see if we uncover some weirdness in ancient libs, since it looks like the odds are very low.
Uh oh!
There was an error while loading.Please reload this page.
Clean up the intent, resulting assembly stays the same as far as I could tell (objdump + vimdiff)
Follow-up#7781
if(s)helper is kind of redundant though, sincec_str()buffer()may never be nullptr and always points to something. Still needed to be compatible though, if something uses it