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

Create empty method for String class #6293

Merged
merged 11 commits into from
Jul 15, 2019
Merged

Create empty method for String class #6293

merged 11 commits into from
Jul 15, 2019

Conversation

ruggi99
Copy link
Contributor

@ruggi99 ruggi99 commented Jul 12, 2019

No description provided.

@earlephilhower
Copy link
Collaborator

@d-a-v, this brings to mind the constant emptyString. Your thoughts?

@earlephilhower earlephilhower requested a review from d-a-v July 12, 2019 21:52
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.

There was an aborted effort #5546 from @jjsuwa to bring global emptyString right into this class.
If it were to happen again, its name would be String::empty.
Thus I'd propose do rename the bool function in this PR to String::isEmpty(), what do you think ?

@jjsuwa
Copy link
Contributor

jjsuwa commented Jul 15, 2019

@d-a-v wrote:

There was an aborted effort #5546 from @jjsuwa to bring global emptyString right into this class.
It it were to happen again, its name would be String::empty.
Thus I'd propose do rename the bool function in this PR to String::isEmpty(), what do you think ?

Of course me too, String::empty() appears for me to be a straightforward misunderstanding of "truncation the string to 0-length".

ruggi99 added 2 commits July 15, 2019 13:13
Changed method name from "empty" to "isEmpty". Created a new method to empty a string
@d-a-v
Copy link
Collaborator

d-a-v commented Jul 15, 2019

@ruggi99
Can you please rename the new String::empty() to String::clear(),
so we can reserve String::empty for another PR (not this one) in which global emptyString would be renamed to String::empty (with changes for all references to emptyString through this repository).

Changed method name from "empty" to "clear".
@ruggi99
Copy link
Contributor Author

ruggi99 commented Jul 15, 2019

I've done. I think setLen(0) is enough to clear a string, isn't it?

@jjsuwa
Copy link
Contributor

jjsuwa commented Jul 15, 2019

@d-a-v wrote:

so we can reserve String::empty for another PR (not this one) in which global emptyString would be renamed to String::empty (with changes for all references to emptyString through this repository).

No excessive consideration about the PR, it's merely proposal :)

@ruggi99
Copy link
Contributor Author

ruggi99 commented Jul 15, 2019

No excessive consideration about the PR, it's merely proposal :)

I think this is a good idea

@d-a-v
Copy link
Collaborator

d-a-v commented Jul 15, 2019

I've done. I think setLen(0) is enough to clear a string, isn't it?

It marks it as empty without any kind of reallocation, I guess this is what to be expected for this call.

No excessive consideration about the PR, it's merely proposal :)

Why has it been closed ?

@jjsuwa
Copy link
Contributor

jjsuwa commented Jul 15, 2019

No excessive consideration about the PR, it's merely proposal :)

Why has it been closed ?

It was premature I guessed.

@devyte devyte merged commit 1c68d02 into esp8266:master Jul 15, 2019
@ruggi99 ruggi99 deleted the string_empty_method branch July 15, 2019 14:59
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.

5 participants