Skip to content

[Proposal] Clarify (the existence of) the constant empty String #5546

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

Closed
wants to merge 3 commits into from

Conversation

jjsuwa
Copy link
Contributor

@jjsuwa jjsuwa commented Dec 23, 2018

instead of use of naked global symbol.

instead of use of naked global symbol.
@jjsuwa jjsuwa force-pushed the clarify_const_empty_String branch from afd32a7 to 9335115 Compare December 23, 2018 23:10
@devyte devyte self-requested a review December 25, 2018 22:44
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.

I like the way this was factored, very clean!

@@ -70,6 +72,9 @@ class String {
explicit String(double, unsigned char decimalPlaces = 2);
~String(void);

// empty string singleton
static constexpr const String& empty = ::__emptyString;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't call the static member "empty". It should be ok to also call it "emptyString", because it is scoped, or call it something else (emptyStr, nullString, defString, etc)

Copy link
Collaborator

@devyte devyte Jan 3, 2019

Choose a reason for hiding this comment

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

Given the deprecation, can this be implemented the other way around? make this member the "real" object, and make the global a reference to this one.

@devyte
Copy link
Collaborator

devyte commented Jan 24, 2019

@jjsuwa ping!

@jjsuwa jjsuwa closed this Feb 4, 2019
@jjsuwa jjsuwa deleted the clarify_const_empty_String branch February 4, 2019 08:39
@d-a-v
Copy link
Collaborator

d-a-v commented Feb 4, 2019

@jjsuwa Are you going to make a new PR for this ?

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