Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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