Skip to content

Base64::encode : const correctness / String by reference passing #6581

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

Merged
merged 2 commits into from
Oct 4, 2019

Conversation

dirkmueller
Copy link
Contributor

This is another instance in the core library 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.

This is another instance in the core library 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.
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 changes LGTM.

The actual base64::encode() function is kind of awful. Figure out how large the result will be, allocate it, write the encoded data to the buffer, and then create another string of the same size (malloc 2x) and memcpy the result, then free fir first buffer.

But that's another PR...

@dirkmueller
Copy link
Contributor Author

@earlephilhower Inoticed the same, but I failed so far to "use" the String buffer for this. Only trick I know is with std::move().

@earlephilhower
Copy link
Collaborator

I think you'd need to make class base64 a friend to get access to the String::wbuffer() (aka writable buffer pointer) method (and String::reserve() the proper length first, of course). Otherwise some sort of new String::assumeBuffer() or something would need to be added. Either would engender more debate than this PR, so I'd worry about it later.

@earlephilhower earlephilhower merged commit bab2880 into esp8266:master Oct 4, 2019
@earlephilhower
Copy link
Collaborator

@dirkmueller I realized you can actually do inline base64 encoding directly into the string w/o any add'l String accessor methods. Just pass in the String and instead of writing bytes to the array, just += the chars as they're generated. Easy-peasy, and 1/2 the RAM needed. But that's another PR. :)

@devyte
Copy link
Collaborator

devyte commented Oct 5, 2019

Don't forget to reserve() beforehand.

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.

3 participants