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: remove operator==(const __FlashStringHelper*) #8569

Merged
merged 8 commits into from
May 17, 2022

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented May 16, 2022

fixes https://gitter.im/esp8266/Arduino?at=627fee0ed30b6b44ebeaa803

@bwjohns4 @JAndrassy there will still be a warning, because nullptr should be used instead of NULL.

@mcspr
Copy link
Collaborator

mcspr commented May 16, 2022

We don't want to overload comparisons though? Check out operator calls you are proposing with older versions, or the esp32 Core, or even the ArduinoCore API test suite.

// with 2.7.4
src/main.cpp: In function 'void setup()':
src/main.cpp:7:16: error: invalid conversion from 'int' to 'const char*' [-fpermissive]
     if (tmp == 5) {
                ^

Main ambiguity comes from fpstr vs. const char, that wasn't there in the older Core versions.
== NULL is still an equals(int), even with the overloaded nullptr you are adding
it is a warning, but not something critical from the gcc pov.

Yes, this is another case of older code breaking, but I'd rather WiFiManager fixes this issue b/c we add another heap of issues on top.

src/main.cpp: In function 'void setup()':
src/main.cpp:7:16: warning: passing NULL to non-pointer argument 1 of 'bool String::operator==(int) const' [-Wconversion-null]
    7 |     if (tmp == NULL) {
      |                ^~~~
In file included from /home/builder/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/Arduino.h:291,
                 from src/main.cpp:1:
/home/builder/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/WString.h:234:30: note:   declared here
  234 |         bool operator ==(int val) const {
      |

cores/esp8266/WString.h Outdated Show resolved Hide resolved
@d-a-v d-a-v marked this pull request as draft May 16, 2022 14:17
Comparison with `P("test")` will use an implicit temporary `String(P("test"))` and restore warning-less & correct comparison with NULL through `==(const char*)` overload.
@d-a-v
Copy link
Collaborator Author

d-a-v commented May 16, 2022

Removing ==(FlashStringHelper*) overload still allows comparison with P("test") which will use an implicit temporary String(P("test")),
and it also restores warning-less & correct comparison with NULL through ==(const char*) overload.

@d-a-v d-a-v marked this pull request as ready for review May 16, 2022 14:33
@mcspr
Copy link
Collaborator

mcspr commented May 16, 2022

note that #8106 used this as a way to shrink ESPeasy binary size due to a temporary String obj

@d-a-v
Copy link
Collaborator Author

d-a-v commented May 16, 2022

note that #8106 used this as a way to shrink ESPeasy binary size due to a temporary String obj

True.
... after checking, the NULL comparison error is existing from this pull request, which happenned after few months 3.0.2 release.

@d-a-v
Copy link
Collaborator Author

d-a-v commented May 16, 2022

With this last commit, NULL is correctly managed, with a deprecating warning so user is invited to switch to nullptr

return length() == 0;
}
[[deprecated("use nullptr instead of NULL")]]
bool operator ==(decltype(NULL)) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just note that the trade-off is someone will write obj == 5; returning false positive :)
(...and iirc ide default to 'no warnings', which does not help...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that std::nullptr_t is a distinguishable type, I tried the same with decltype(NULL).
Despite the existence of the __null internal keyword and the __sentinel__ attribute, I currently cannot manage NULL separately from int/long.

I reverted: ==(FlashStringHelper*) is removed again.

We still have this:
#define NULL nullptr

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think they refer to some old version of GCC (pre 4.6?) with the __null stuff.
And I'd like to find some examples of #define NULL std::nullptr, as it seems like something for the system header to have

Copy link
Collaborator Author

@d-a-v d-a-v May 17, 2022

Choose a reason for hiding this comment

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

The only one I could find that is installed on my workstation and probably rarely used(*).

(*)given std:: and using are both lacking

Copy link
Collaborator

@mcspr mcspr May 17, 2022

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

cores/esp8266/WString.h Outdated Show resolved Hide resolved
@d-a-v
Copy link
Collaborator Author

d-a-v commented May 17, 2022

This change proposal ends up with simply removing operator==(FlashStringHelper).

@mcspr mcspr changed the title WString: overload == WString: remove operator==(const __FlashStringHelper*) May 17, 2022
@mcspr
Copy link
Collaborator

mcspr commented May 17, 2022

renamed
str == F(...) alternative would be str == PSTR(...) accepting progmem'ed strings
pointer coercion will drive us mad otherwise

@d-a-v d-a-v merged commit fbba25c into esp8266:master May 17, 2022
@d-a-v d-a-v deleted the streqeq branch May 17, 2022 12:58
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
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