Skip to content

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

Merged
earlephilhower merged 7 commits intoesp8266:masterfrom
paulocsanz:master
Nov 13, 2020
Merged

Provide String::indexOf for a char* needle#7706
earlephilhower merged 7 commits intoesp8266:masterfrom
paulocsanz:master

Conversation

@paulocsanz
Copy link
Contributor

@paulocsanz paulocsanz commented Nov 13, 2020

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.

@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

paulocsanz commented Nov 13, 2020

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!

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