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

WString: unify numeric conversion and fix assignments #8526

Merged
merged 9 commits into from Apr 5, 2022

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Mar 30, 2022

Restore the pre-3.0.0 behaviour when we could assign numeric values to
the string object. After introducing operator =(char), everything was
converted to char instead of the expected 'stringification' of the
number (built-in int, long, unsigned int, unsigned long, long long,
unsigned long long, float and double)

Add toString() that handles conversions, re-use it through out the class

Fix #8430

Restore the pre-3.0.0 behaviour when we could assign numeric values to
the string object. After introducing `operator =(char)`, everything was
converted to `char` instead of the expected 'stringification' of the
number (built-in int, long, unsigned int, unsigned long, long long,
unsigned long long, float and double)
@mcspr
Copy link
Collaborator Author

mcspr commented Mar 30, 2022

I suppose there should be tests for assignment... Since there are for constructors.
Tests added

… decimalPlaces

branch via if (base == 10) { sprintf(...) } else { ... } instead of separate funcs
reuse the constructor for numeric types where it's possible
nonstd arduino funcs expect this usage pattern
Comment on lines -67 to -77
explicit String(unsigned char, unsigned char base = 10);
explicit String(int, unsigned char base = 10);
explicit String(unsigned int, unsigned char base = 10);
explicit String(long, unsigned char base = 10);
explicit String(unsigned long, unsigned char base = 10);
explicit String(long long /* base 10 */);
explicit String(long long, unsigned char base);
explicit String(unsigned long long /* base 10 */);
explicit String(unsigned long long, unsigned char base);
explicit String(float, unsigned char decimalPlaces = 2);
explicit String(double, unsigned char decimalPlaces = 2);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is mostly for verbosity. Due to the amount of magic ctors we have... we can't really have std::initializer_list<char> ctor as well, since everything would be converted into it

e.g.

String(std::initializer_list<char> data) {
    concat(data.begin(), data.size());
}

So only purpose of the below is to allow to omit String{...} in front of the arguments list. Where we suppose to have explicit for String(int, base) overload

core/test_string.cpp:620:51: error: converting to ‘String’ from initializer list would use explicit constructor ‘String::String(int, unsigned char)’
  620 |     String strings[] {{62, 10}, {63, 10}, {64, 10}};
      |                                                   ^

Could be reverted though.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

Tests are nice and seem to cover most of the cases, if not all.
Thanks to restore the API and factorize code

@mcspr mcspr merged commit 584d2f2 into esp8266:master Apr 5, 2022
@mcspr mcspr deleted the arduino-string-can-assign-stuff branch April 5, 2022 12:31
@mcspr mcspr linked an issue Apr 11, 2022 that may be closed by this pull request
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.

Wrong value on EEPROM.Read since 3.0.0 core arduino version String: API must be fixed
2 participants