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

Avoid writing to const storage #8463

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Avoid writing to const storage #8463

merged 1 commit into from
Jan 28, 2022

Conversation

paulocsanz
Copy link
Contributor

Fixes #8462

This avoids the null termination requirement of both String::substring and String::lastIndexOf by using APIs that don't require it. So we can stop writing to the buffer inside of const functions.

There might be a off by one error there, I need to take a better look at the code and would love if someone could review.

@earlephilhower
Copy link
Collaborator

Seems to have broken some host tests:

-------------------------------------------------------------------------------
String SSO works
-------------------------------------------------------------------------------
core/test_string.cpp:322
...............................................................................

core/test_string.cpp:420: FAILED:
  REQUIRE( s.length() == 5 )
with expansion:
  15 == 5

@earlephilhower
Copy link
Collaborator

s = "0123456789abcde";
s = s.substring(s.indexOf('a'));
REQUIRE(s == "abcde");
REQUIRE(s.length() == 5);

@paulocsanz
Copy link
Contributor Author

Will fix it, thanks! How can I run the host tests locally so I don't clutter the project's CI and can iterate over the code faster?

@earlephilhower
Copy link
Collaborator

Will fix it, thanks! How can I run the host tests locally so I don't clutter the project's CI and can iterate over the code faster?

In tests/host just run make test. You'll probably need valgrind and a couple other dependencies which will show up.

@paulocsanz
Copy link
Contributor Author

Hmm, I'm getting a bunch of compilation errors because of std::enable_if_t, std::is_same_v, std::decay_t and #include "../lib/littlefs/lfs.h, but it seems I fixed the test.

@mcspr
Copy link
Collaborator

mcspr commented Jan 25, 2022

Were you intending to fix wbuffer() as well? It seems the const_cast originates b/c of the const qualifier(s), where the compiler did warn about the const misnomer but it was circumvented

cores/esp8266/WString.cpp Outdated Show resolved Hide resolved
@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 25, 2022

Yeah, I would prefer to make wbuffer into non const, should I change it in this PR too then?

I will mess with this PR's code after work today.

@paulocsanz
Copy link
Contributor Author

paulocsanz commented Jan 26, 2022

@d-a-v Done, your trick worked!

I also changed wbuffer to make it non const.

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.

LGTM

@mcspr mcspr merged commit 9f536e6 into esp8266:master Jan 28, 2022
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.

Avoid writing in const functions, as it can technically be UB
4 participants