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

String: compatibility with 64 bits scalars #7863

Merged
merged 9 commits into from
Feb 13, 2021

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Feb 7, 2021

time_t is now 64 bits.
This PR resolves String(time_t) ambiguity.

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

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

Also note of the charconv in the stdlib (and it's source as well)
https://en.cppreference.com/w/cpp/utility/to_chars
https://en.cppreference.com/w/cpp/string/basic_string/to_string

lgtm as-is, but String is inherently weird with it's approach to conversions :/

cores/esp8266/stdlib_noniso.h Outdated Show resolved Hide resolved
cores/esp8266/WString.cpp Show resolved Hide resolved
@d-a-v
Copy link
Collaborator Author

d-a-v commented Feb 10, 2021

@mcspr I tried with charconv that I didn't know. That's very handy !
That one is faster (1.6x) but has a cost (+880B/flash+256B/rodata) (compared against lltoa)*
It has to be noted that these functions charconv(std::to_chars) or lltoa are used only when radix is not 10 (in which case sprintf is used). It should be quite rare.
Without further comments, I will push a newer commit keeping only lltoa. These functions will anyway not be linked in the general case.

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.

None yet

3 participants