NotificationsYou must be signed in to change notification settings
Fork13.3k
Star16.5k

String: compatibility with 64 bits scalars#7863

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

Jump to bottom

Merged

earlephilhower merged 9 commits intoesp8266:masterfromd-a-v:StringLL

Feb 13, 2021

Merged

String: compatibility with 64 bits scalars#7863

earlephilhower merged 9 commits intoesp8266:masterfromd-a-v:StringLL

Feb 13, 2021

Conversation

Copy link

Collaborator

d-a-v commentedFeb 7, 2021•
edited
Loading

time_t is now 64 bits.
This PR resolvesString(time_t) ambiguity.

d-a-v added4 commits

February 7, 2021 01:28

String: compatibility with 64 bits scalars

fa09a50

time_t is now 64 bits. Strig(time_t) was ambiguoustests added

use a single div+mod facility

8c4c363

fix host tests

598491f

remove generic udivmod

fe0b14c

earlephilhower approved these changes

Feb 7, 2021

View reviewed changes

Copy link

Collaborator

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!

Merge branch 'master' into StringLL

944e2e8

mcspr reviewed

Feb 8, 2021

View reviewed changes

Copy link

Collaborator

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 thecharconv 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 OutdatedShow resolvedHide resolved

cores/esp8266/WString.cppShow resolvedHide resolved

radix type, test with charconv

54bbe23

Copy link

CollaboratorAuthor

d-a-v commentedFeb 10, 2021•
edited
Loading

@mcspr I tried withcharconv 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 functionscharconv(std::to_chars) orlltoa are used only when radix is not 10 (in which casesprintf is used). It should be quite rare.
Without further comments, I will push a newer commit keeping onlylltoa. These functions will anyway not be linked in the general case.