Skip to content

String: compatibility with 64 bits scalars#7863

Merged
earlephilhower merged 9 commits into
esp8266:masterfrom
d-a-v:StringLL
Feb 13, 2021
Merged

String: compatibility with 64 bits scalars#7863
earlephilhower merged 9 commits into
esp8266:masterfrom
d-a-v:StringLL

Conversation

@d-a-v
Copy link
Copy Markdown
Collaborator

@d-a-v d-a-v commented Feb 7, 2021 •
edited
Loading

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

earlephilhower
earlephilhower approved these changes Feb 7, 2021
View reviewed changes
Copy link
Copy Markdown
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!

mcspr
mcspr reviewed Feb 8, 2021
View reviewed changes
Copy link
Copy Markdown
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 :/

Comment thread cores/esp8266/stdlib_noniso.h Outdated
Comment thread cores/esp8266/WString.cpp
@d-a-v
Copy link
Copy Markdown
Collaborator Author

d-a-v commented Feb 10, 2021 •
edited
Loading

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

earlephilhower
earlephilhower reviewed Feb 11, 2021
View reviewed changes
Comment thread cores/esp8266/WString.cpp Outdated
@earlephilhower earlephilhower merged commit 483519f into esp8266:master Feb 13, 2021
@d-a-v d-a-v mentioned this pull request Feb 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@earlephilhower earlephilhower earlephilhower approved these changes

@mcspr mcspr mcspr left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@d-a-v @earlephilhower @mcspr