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

Pass String by const reference [3.0] #6583

Merged
merged 2 commits into from
Jul 10, 2020

Conversation

dirkmueller
Copy link
Contributor

Passing String by value means a full copy-constructor/temporary
string creation which is slightly inefficient. Avoid that
by using const references.

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.

These are all basically private inside ESP8266WebServer, so I don't see any issue w/user classes even though they are virtual methods.

@devyte
Copy link
Collaborator

devyte commented Oct 3, 2019

The reason they're virtual is to allow a user to implement his own RequestHandler objects.
Target must be v3.

@devyte devyte added this to the 3.0.0 milestone Oct 3, 2019
@earlephilhower
Copy link
Collaborator

Actually, @devyte, while what you say is possible, IMO they're virtual because we need to store a pointer to a generic RequestHandler in the webserver (i.e. internal considerations, not custom overloads).

Overloaded classes would suffer object slicing if we stored a pointer to the base class in the WebServer object, so these functions need to be virtual (even though I was able to rewrite the WebServer class to be a template w/o virtuals).

Passing String by value means a full copy-constructor/temporary
string creation which is slightly inefficient. Avoid that
by using const references.
bool handle(WebServerType& server, HTTPMethod requestMethod, String requestUri) override {
bool handle(WebServerType& server, HTTPMethod requestMethod, const String& __requestUri) override {
String requestUri(__requestUri);

Copy link
Collaborator

Choose a reason for hiding this comment

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

This String copy can be handled differently in a further fixing commit.

@d-a-v d-a-v merged commit 83158af into esp8266:master Jul 10, 2020
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.

4 participants