Further const correctness / String by reference passing cleanups

[dirkmueller]

There are actually several instances where we pass in read-only
parameters as pass-by-value, where in the case of String() that
is inefficient as it involves copy-constructor/temp string creations.

We can avoid that, similarly to single character string concatenations
done via string literals instead of char literals.

This is a continuation of #6569.

[earlephilhower]

Most CI fails are due to arduino-builder crapping out, but the PlatformIO tests do show problems that need fixing in the examples to make this patch work.

https://travis-ci.org/esp8266/Arduino/jobs/591524978

https://travis-ci.org/esp8266/Arduino/jobs/591524979

[dirkmueller]

yep, I am still trying to learn how to replicate what travis is testing locally so that I can fix all of them in one go. for now this is an iterative approach :)

[d-a-v]

cd tests
./run_CI_locally.sh

(edit: submodules are now automatically updated)

[earlephilhower]

Looks good and makes the interfaces more rational for a lot of things involving String parameters.

[devyte]

Careful here. I'm all for constness, but we've been bitten before due to differences vs. the Arduino reference, where constness isn't always the best. Specifically, differences in constness vs. the Arduino reference can cause grief for 3rd party lib maintainers.
Please check whether any of the interfaces here are part of the Arduino reference (I didn't look through this whole PR), and if so whether the constness is different. In such cases, if any, I suggest reverting. If there are no such cases, please confirm that and I'll approve.

[dirkmueller]

@devyte all API changes are in classes where their name starts with ESP8266. I would assume there is no 3rd party incompatibility here. There might be corner cases where ESP8266 code is affected, but I can't imagine this to be a very frequent case (to be honest I don't even know how you would notice String vs const String& other than reduced code footprint and better performance).

[d-a-v]

About CI, you can run mock trivial test locally with

cd tests/host
make clean
make  ../../libraries/ESP8266WebServer/examples/FSBrowser/FSBrowser

(from tests/buildm.sh)

[earlephilhower]

@devyte, the only changes I see in the externally visible *.h files are:

• type varname -> const type varname
• String x -> const String &x

Adding a const to the param seems absolutely safe, as the const flows down into the function but not upwards to the caller, right? A char* can be passed into a fcn requiring a const char * without any concern.

The & operator is a compiler structure hint, and other than passing by ref and not value user code should need no change, either. The compiler will simply emit a push addr instead of memcpy onto stack.

Granted, if you have a blob using these libs the 2nd change will cause trouble, but the ESPRESSIF ones do not use our libs, obviously and we've never guaranteed binary ABI compatibility.

[devyte]

The constness trouble was encountered by @d-a-v and was related to 3rd party libs that are meant to work across cores for diffrent uCs. In the end, he had to roll back.
Example: derive a class from one of these and override a method. In our core it's const, in some other core it's not const => 3rd party lib no longer compiles for both cores and requires extensive (additional) #ifdefing.

Like @dirkmueller said, these have the esp8266 prefix and I suspect the signature changes might be ok, but I don't see any assurance, which means this could be a breaking change.
I'm ok with moving forward, as long as the risk is understood: if a breakage report comes in this will need to be rolled back, and if there is a release in the middle, we'll have to make a new release with the rollback fix.

[earlephilhower]

Ah, I see. The code as-is is safe, but there might be libs which might subclass them. IIRC, @d-a-v's issue was when he adjusted Arduino Client(?) and other libs which were subclasses of it crapped out.

[devyte]

On further thought, this is a breaking change any way you look at it, because any user with derived classes that override methods will need updating to add constness. I say target for v3.

[dirkmueller]

@d-a-v overrides are only for virtual methods, which most of them are not. are you okay when we remove that change for public/protected virtual methods?

I think the only part that is a change in a virtual function signature is in RequestHandler.h which I think is actually unreleased. I'll double check this part.

[d-a-v]

My issues were mostly because of inheritance.
I don't think String is inherited by other projects.
That kind of changes is IMHO harmless when the class is used (vs inherited). And useful :)

[dirkmueller]

sorry, wrong completion, I wanted to talk to @devyte

[d-a-v]

No harm, I was answering to @earlephilhower two posts earlier 😆
Anyway I don't see any "official" arduino classes in this PR

[devyte]

@dirkmueller I think so, but I don't want to just drop the rest either. Please split the virtual ones into a separate pr, and I'll target for v3.

[dirkmueller]

@devyte splitted into #6583