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

Test: fixing itoa implementation and clean-up of tests and test Makefile #8531

Merged
merged 3 commits into from
Apr 11, 2022

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Apr 6, 2022

Update itoa to be the same as newlib, also fixing edgecase of abs(INT_MIN)
Update WString.cpp:toString() integer conversions to use noniso funcs
Update WString to use C++ limits lib
Remove legacy gcc versions from Makefile and allow overrides
Don't fallback to c11 and c++11, source cannot support that

Additionally, allow test binary to accept arguments

~/d/e/t/host> make TEST_ARGS=[core][String] test
Makefile:31: using g++
Makefile:46: Cannot compile in 32 bit mode (g++-multilib is missing?), switching to native mode
Makefile:57: compiling in native mode
/home/runner/dev/esp8266-Arduino/tests/host/bin/host_tests [core][String]
===============================================================================
All tests passed (288 assertions in 20 test cases)

As mentioned by @d-a-v in the Matrix channel
Not really sure if it's the best idea feature-wise, but it keeps this weird Arduino legacy output throughout the class. Like, the way -100 with base 16 gets converted into ffffff9c instead of -64 (which what I'd expect here, with any other sane API :). Main reason for that is Arduino API does printf equivalent of %X. But, it is consistent with other implementations, and there's no branching required to the printf
ref. https://github.com/arduino/ArduinoCore-API/blob/master/test/src/itoa.cpp, https://github.com/espressif/arduino-esp32/blob/6cfe4613e4b4846e1ab08c7f78b7ea241f52c7da/cores/esp32/WString.cpp#L80-L89

Just using the noniso funcs saves a little bit of code. For example checking on the nm output for int ctor when building the test binary

- 0000000000000150 T String::String(int, unsigned char)
+ 0000000000000121 T String::String(int, unsigned char)

I'd like to run these on the board before pushing though, since I have only tried the host test

Update itoa to be the same as newlib, fixing edgecase of abs(INT_MIN)
Update WString.cpp:toString() integer conversions to use noniso funcs
Remove legacy gcc versions from Makefile and allow overrides
Don't fallback to c11 and c++11, source cannot support that
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.

Without actually trying, it looks good.
Thanks !

@mcspr mcspr merged commit 520233f into esp8266:master Apr 11, 2022
@mcspr mcspr deleted the noniso-is-weird branch April 11, 2022 10:53
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
…ile (esp8266#8531)

* Test: fixing itoa implementation, clean-up of tests and test runner

Update itoa to be the same as newlib, fixing edgecase of abs(INT_MIN)
Update WString.cpp:toString() integer conversions to use noniso funcs
Remove legacy gcc versions from Makefile and allow overrides
Don't fallback to c11 and c++11, source cannot support that

* CXX and CC are make predefined values, assuming ?= does not work (?)
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.

2 participants