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.

Sign up for GitHub

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

Jump to bottom

Provide String::indexOf for a char* needle #7706

Merged
merged 7 commits into from
Nov 13, 2020
Merged

Provide String::indexOf for a char* needle #7706

merged 7 commits into from
Nov 13, 2020

Conversation

paulocsanz
Copy link
Contributor

Fixes #7705

@paulocsanz paulocsanz changed the title Provide String::indexOf for char* needle Provide String::indexOf for a char* needle Nov 13, 2020
earlephilhower
earlephilhower requested changes Nov 13, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Thanks for not only filing an issue, but proposing a solution too!

You mentioned progmem in your issue and changing to strstr_Pwill make it "just work" with RAM or PROGMEM strings.

cores/esp8266/WString.cpp Outdated Show resolved Hide resolved
@paulocsanz
Add String::indexOf for __FlashStringHelper pointers
c2017d0 
strstr_P documentation says only s2 must be from PROGMEM, so we are
safe.
@paulocsanz
Copy link
Contributor Author

paulocsanz commented Nov 13, 2020 •
edited
Loading

I've went ahead and implemented the changes. Should I change all strstr calls to strstr_P then? The documentation doesn't say anything about passing a needle to strstr_P that does not live in program memory.

But again I'm not an expert in this ecosystem.

Btw did I break anything? The CI seems to have errored (I may have hurt their feelings by force pushing too much lol).

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 13, 2020 •
edited
Loading

Btw did I break anything? The CI seems to have errored (I may have hurt their feelings by force pushing too much lol).

Yes

(click on "details" on any CI line)

edit: It is quite never needed to force-push. Pushing is OK, pulling is needed before pushing if the branch has been updated from github. That way, we have history. The history is anyway squashed at merge time on this repository.

@paulocsanz
Copy link
Contributor Author

Thank you @d-a-v! I've just fixed it.

d-a-v
d-a-v approved these changes Nov 13, 2020
View reviewed changes
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

earlephilhower
earlephilhower requested changes Nov 13, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Other than the single const change noted below, LGTM. Thx!

cores/esp8266/WString.cpp Outdated Show resolved Hide resolved
earlephilhower
earlephilhower approved these changes Nov 13, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Thx! LGTM.

@earlephilhower earlephilhower merged commit c5c9f84 into esp8266:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@earlephilhower earlephilhower earlephilhower approved these changes

@d-a-v d-a-v d-a-v approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to call String::indexOf with char* for a needle
3 participants
@paulocsanz @d-a-v @earlephilhower