[String] Fix float assignment using wrong type #8663
Conversation
Using 3.0.2 assigning a float to a string resulted in undefined behaviour. ```c++ String str; float myfloat[12][4]; myfloat[0][0] = 1.0f; str = myfloat[0][0]; ``` `str` does contain a single char. Worked fine in 2.7.4
|
Hmm @tonhuisman mentioned to me that the operator== for flash strings was removed in another commit, I started looking for it in the history. It was indeed removed here: #8569 |
|
You sure about the source of the issue? What about a device / host test? Notice the CI suite and the |
...and are these tests done on git installation? |
Used this in the PlatformIO config: I did for sure a clean build. This is the section which was our use case: By explicitly creating a string and assigning it to the N.B. the operator== doesn't have anything to do with this fix, it was just added as way to reduce build size, but I guess if it does cause issues, I will remove it and go over my code to replace all == and != compares with flash strings to the equals function. |
With the
The idea is to have some small test code we could run without the whole ESPeasy becoming a test case |
I did edit the WString.cpp/.h files in my local branch of this repo and then copied those 2 files to the tree in the .pio/packages folder. I also removed all pio related caches before testing.
Will try to make a suitable test case later this evening. |
|
Why I am asking, I don't see any issues with overload itself (that this PR proposes to change) PIO repo for me is in [env]
framework = arduino
[env:d1_mini]
platform = espressif8266
board = d1_mini
build_flags = -g
platform_packages =
mcspr/toolchain-xtensa @ ~5.100300.211127
platformio/framework-arduinoespressif8266 @ git+https://github.com/esp8266/Arduino.git#include <Arduino.h>
void setup() {
Serial.begin(115200);
}
void loop() {
String a(1.0f);
printf("String ctor -> \"%s\"\n", a.c_str());
String b;
float num = 1.0f;
b = num;
printf("String assignment -> \"%s\"\n", b.c_str());
delay(1000);
} |
|
OK, made a quick MCVE to show what's wrong. #define COL 12
#define ROW 4
String str;
float myfloat[COL][ROW];
int idx;
void setup() {
idx = 0;
Serial.begin(115200);
for (int i = 0; i < COL; ++i) {
for (int j = 0; j < ROW; ++j) {
myfloat[i][j] = 1.0f * i * j + j;
}
}
}
void loop() {
str = myfloat[idx / COL][idx % ROW];
Serial.print(F("str: '"));
Serial.print(str);
Serial.println('\'');
delay(1000);
++idx;
if (idx >= (COL*ROW)) {
idx = 0;
}
} |
|
OK, I did check the content of WString.h/.cpp and indeed these are not using the latest versions. |
|
The idea here is to use It should show up in the PACKAGES list when But, it is up to the user to manually keep it up to date when no commit or version (tag) are specified in the repo URI idk how GUI / IDE interacts with updates, and whether there are any more commands or command line flags that help out with the internal package structure While I simply get by and edit package files and maintain development branches right there at the path I mentioned above, it is also possible to use 'local folders / symlinks' (https://docs.platformio.org/en/latest/core/userguide/pkg/cmd_install.html?highlight=symlink#local-folder) platform_packages =
framework-arduinoespressif8266 @ symlink://c:/Users/you/Documents/PlatformIO/esp8266-ArduinoOr, I wonder it would be best to refer to specific commit / tag / ref via github built-in repo download feature. No confusion in versioning then, everyone building ESPeasy gets the same version |
|
@mcspr Now it is working in the MCVE code without the needed changes of this PR. N.B. my suggested fix may also cause issues for other types like |
Using 3.0.2 assigning a float to a string resulted in undefined behaviour.
strdoes contain a single char.Not sure if a 2D float array is needed to reproduce it, but that's how it was used in my project.
These were part of a larger struct, so maybe the assignment should be done in a separate function to make sure the compiler is not optimizing the code too much.
Worked fine in 2.7.4
Looks like it might have been introduced here: #8526 @mcspr
Change is mainly to enforce
explicitbehavior when calling thetoStringstatic functions by using const reference as their argument.Also added
bool operator ==(const __FlashStringHelper *s) constand its counter part, which saved roughly 3k in build size on my project ESPEasy.