Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
(As discussed during lunch...) Why not just do this in this case and remove
EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
I forget if this runs the constructor on first use or when the app starts
up. If it's the latter and you care, you can even just make it a null
pointer and allocate it on first use.
No matter what PSA or comments you write, someone will use it again if it's
there.
J
On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting pkast...@google.com wrote:
If you have ever used any of the EmptyXXX() functions, or ever will, please
read on.
These functions (in string_util.h and gurl.h) are meant for a single,
specific use case:
const std::string MyClass::foo() const {
return (everything == OK) ? member_string : EmptyString();
}
Here you cannot return string(), because it's destroyed before the
function returns, and the caller receives garbage; and you don't want to
have the function return by value, because you can access the member
variable directly and save a copy. The utility functions give you a global
empty string that you can safely return a const reference to.
DON'T USE THESE OUTSIDE THIS CASE. You should never use these as
initializers, arguments to functions, or return values in functions that
return by value. Just use the default constructor; that's what it's there
for.
I have a change out for review that removes the erroneous uses of these
from our codebase and clarifies the comment on their declaration, but it's
worth calling this out directly so they don't creep back in.
PK
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow jor...@chromium.org wrote:
(As discussed during lunch...) Why not just do this in this case and
remove EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
This is illegal per the Google style guide:
Objects with static storage duration, including ... function static
variables, must be Plain Old Data (POD): only ints, chars, floats, or
pointers, or arrays/structs of POD. ... This rule completely disallows
vector (use C arrays instead), or string (use const char []).
If you see code like this in our codebase, please fix it to not use
static/global non-POD types.
PK
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 1:38 PM, Peter Kasting pkast...@google.com wrote:
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow jor...@chromium.org wrote:
(As discussed during lunch...) Why not just do this in this case and
remove EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
This is illegal per the Google style guide:
Objects with static storage duration, including ... function static
variables, must be Plain Old Data (POD): only ints, chars, floats, or
pointers, or arrays/structs of POD. ... This rule completely disallows
vector (use C arrays instead), or string (use const char []).
If you see code like this in our codebase, please fix it to not use
static/global non-POD types.
You ignored the second half of my suggestion.
IIRC there's a macro (maybe it was only in WebKit) to get around this issue
as well.
J
PK
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
What about renaming the function? EmptyStringHACK() or something? On Thu, Jan 7, 2010 at 1:49 PM, Peter Kasting pkast...@google.com wrote: On Thu, Jan 7, 2010 at 1:43 PM, Jeremy Orlow jor...@chromium.org wrote: You ignored the second half of my suggestion. The second half of your suggestion leaks memory. When we have easy and elegant ways to avoid memory leaks, it behooves us to use them. It also seems like a poor idea to me to suggest that, potentially, any function returning a string by reference might have to have its own memory leak, or allocation code, or static object, if it needs to be able to return an empty object. Even if we could do that with no ill consequences, it would be nice to avoid it. After my patch, the total number of calls of these functions in the entire codebase is something like 10 instances. They're rare enough to be invisible to most people and unusual otherwise. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 1:50 PM, Jeremy Orlow jor...@chromium.org wrote: What about renaming the function? EmptyStringHACK() or something? It's not a hack. It's a perfectly legitimate thing to use, and not something we're going to get rid of, unlike ToWStringHack(). Darin suggested we could make these return const pointers instead of const refs, so callers would need to explicitly deref, to make things look uglier. I'm not a big fan of this. If someone does misuse one of these, it won't corrupt memory, so it's not catastrophe. Removing all the wrong uses and adding clear comments about the right uses, here and in the code, seems sufficient to me. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow jor...@chromium.org wrote:
(As discussed during lunch...) Why not just do this in this case and
remove EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
This is not thread safe. EmptyString() uses SingletonT to basically
achieve the same thing in a thread safe manner.
-Darin
I forget if this runs the constructor on first use or when the app starts
up. If it's the latter and you care, you can even just make it a null
pointer and allocate it on first use.
No matter what PSA or comments you write, someone will use it again if it's
there.
J
On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting pkast...@google.com wrote:
If you have ever used any of the EmptyXXX() functions, or ever will,
please read on.
These functions (in string_util.h and gurl.h) are meant for a single,
specific use case:
const std::string MyClass::foo() const {
return (everything == OK) ? member_string : EmptyString();
}
Here you cannot return string(), because it's destroyed before the
function returns, and the caller receives garbage; and you don't want to
have the function return by value, because you can access the member
variable directly and save a copy. The utility functions give you a global
empty string that you can safely return a const reference to.
DON'T USE THESE OUTSIDE THIS CASE. You should never use these as
initializers, arguments to functions, or return values in functions that
return by value. Just use the default constructor; that's what it's there
for.
I have a change out for review that removes the erroneous uses of these
from our codebase and clarifies the comment on their declaration, but it's
worth calling this out directly so they don't creep back in.
PK
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 2:46 PM, Darin Fisher da...@chromium.org wrote:
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow jor...@chromium.org wrote:
(As discussed during lunch...) Why not just do this in this case and
remove EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
This is not thread safe. EmptyString() uses SingletonT to basically
achieve the same thing in a thread safe manner.
Is there something wrong with returning by copy, and relying on the compiler
to execute a return value optimization?
-Albert
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 2:53 PM, Victor Khimenko k...@google.com wrote:
On Fri, Jan 8, 2010 at 1:51 AM, Albert J. Wong (王重傑)
ajw...@chromium.orgwrote:
On Thu, Jan 7, 2010 at 2:46 PM, Darin Fisher da...@chromium.org wrote:
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow jor...@chromium.orgwrote:
(As discussed during lunch...) Why not just do this in this case and
remove EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
This is not thread safe. EmptyString() uses SingletonT to basically
achieve the same thing in a thread safe manner.
Is there something wrong with returning by copy, and relying on the
compiler
to execute a return value optimization?
Do you volunteer to rewrite the compilers to implement said optimization?
GCC, for one will not do so.
According to wikipedia,
http://en.wikipedia.org/wiki/Return_value_optimization, msvc, g++, and icc,
all support it...or am I missing something about this situation that makes
RVO inapplicable?
-Albert
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 2:50 PM, Albert Wong (王重傑) ajw...@google.com wrote:
Is there something wrong with returning by copy, and relying on the
compiler to execute a return value optimization?
I'm not totally sure what your comment is saying.
If you are saying that everywhere in the code can return by value instead of
by const ref and the compiler will optimize it equivalently, you are wrong;
I was under the same misapprehension until yesterday. We've verified that
even in the best case (full optimizations, all functions visible to the
compiler, simple bodies), returning a member by value instead of by const
ref takes more code.
If you are saying that the RVO exists and matters, then of course you're
correct. When you write code like this:
std::string foo() {
std::string a;
// Calculations with a
return a;
}
...The compiler uses the RVO to avoid copying |a| to the return value at
EOF, and instead just allocate |a| directly to the return slot. This is why
we prefer return-by-value to return-by-outparam where possible: RVO makes it
just as cheap, and clearer. But neither is as cheap as return-by-const-ref
if you've already got the referenced object sitting around, as you do on
class member accessors; it's one copy versus zero.
PK
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
[ resending from correct e-mail ] If you are saying that everywhere in the code can return by value instead of by const ref and the compiler will optimize it equivalently, you are wrong; I was under the same misapprehension until yesterday. We've verified that even in the best case That's what I was indeed what I was thinking, and apparently, it was a bad assumption. :-/ Thanks for the explanation. -Albert -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting pkast...@google.com wrote:
If you have ever used any of the EmptyXXX() functions, or ever will, please
read on.
These functions (in string_util.h and gurl.h) are meant for a single,
specific use case:
const std::string MyClass::foo() const {
return (everything == OK) ? member_string : EmptyString();
}
Here you cannot return string(), because it's destroyed before the
function returns, and the caller receives garbage; and you don't want to
have the function return by value, because you can access the member
variable directly and save a copy. The utility functions give you a global
empty string that you can safely return a const reference to.
DON'T USE THESE OUTSIDE THIS CASE. You should never use these as
initializers, arguments to functions, or return values in functions that
return by value. Just use the default constructor; that's what it's there
for.
Out of curiosity, what is wrong with using EmptyString() in those
cases? Is there a correctness problem? Unnecessary inclusion of
string_util.h?
- a
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 3:45 PM, Aaron Boodman a...@google.com wrote: Out of curiosity, what is wrong with using EmptyString() in those cases? Is there a correctness problem? Unnecessary inclusion of string_util.h? There are a couple reasons. Code clarity and consistency is a primary one; using EmptyString() implies you _need_ EmptyString() and that the default constructor won't work, which can be confusing. (I am especially frustrated with code like std::string x(EmptyString());.) For folks used to using WebKit string classes, which differentiate between empty and NULL, this can look like you're saying more about the semantics of the statement than you actually are. If you see code with hundreds of EmptyString()s you begin wondering whether your code should use it too. Another reason is that EmptyString() does a threadsafe access to a global object, whereas the default constructor is a comparatively simple block of inlinable code that the compiler understands and may frequently be able to optimize better. There is not a correctness issue, which is why uses of this can seep into the codebase without people noticing. It is, of course, nice to avoid including string_util.h as you mention, though I'd consider that a minor benefit. The totality of the reasons is somewhat lost (to me at least) in the mists of time; EmptyString() dates from the earliest days of the codebase, and at the time we went round and round a number of times to determine the best solutions for each situation. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 3:45 PM, Aaron Boodman a...@google.com wrote:
On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting pkast...@google.com wrote:
If you have ever used any of the EmptyXXX() functions, or ever will,
please
read on.
These functions (in string_util.h and gurl.h) are meant for a single,
specific use case:
const std::string MyClass::foo() const {
return (everything == OK) ? member_string : EmptyString();
}
Here you cannot return string(), because it's destroyed before the
function returns, and the caller receives garbage; and you don't want to
have the function return by value, because you can access the member
variable directly and save a copy. The utility functions give you a
global
empty string that you can safely return a const reference to.
DON'T USE THESE OUTSIDE THIS CASE. You should never use these as
initializers, arguments to functions, or return values in functions that
return by value. Just use the default constructor; that's what it's
there
for.
Out of curiosity, what is wrong with using EmptyString() in those
cases? Is there a correctness problem? Unnecessary inclusion of
string_util.h?
probably just a tad more costly since it involves SingletonT. but, i
think peter's main reason was simply to stick to the simpler and more
familiar std::string.
-darin
- a
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Fri, Jan 8, 2010 at 2:10 AM, Albert J. Wong (王重傑) ajw...@chromium.orgwrote:
According to wikipedia,
http://en.wikipedia.org/wiki/Return_value_optimization, msvc, g++, and
icc, all support it...or am I missing something about this situation that
makes RVO inapplicable?
Yes. Starting from version 3.1: http://gcc.gnu.org/gcc-3.1/changes.html
G++ now supports the named return value optimization: for code like
A f () {
A a;
...
return a;
}
G++ will allocate a in the return value slot, so that the return becomes a
no-op. For this to work, all return statements in the function must return
the same variable
The limitation is still true AFAIK. Many compilers give up early in case
where function can return two objects depending on some condition.
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Fri, Jan 8, 2010 at 1:51 AM, Albert J. Wong (王重傑) ajw...@chromium.orgwrote:
On Thu, Jan 7, 2010 at 2:46 PM, Darin Fisher da...@chromium.org wrote:
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow jor...@chromium.org wrote:
(As discussed during lunch...) Why not just do this in this case and
remove EmptyString() altogether?
const std::string MyClass::foo() const {
static std::string empty = EmptyString();
return (everything == OK) ? member_string : empty;
}
This is not thread safe. EmptyString() uses SingletonT to basically
achieve the same thing in a thread safe manner.
Is there something wrong with returning by copy, and relying on the
compiler
to execute a return value optimization?
Do you volunteer to rewrite the compilers to implement said optimization?
GCC, for one will not do so.
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 4:02 PM, Darin Fisher da...@chromium.org wrote:
On Thu, Jan 7, 2010 at 3:45 PM, Aaron Boodman a...@google.com wrote:
On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting pkast...@google.com
wrote:
If you have ever used any of the EmptyXXX() functions, or ever will,
please
read on.
These functions (in string_util.h and gurl.h) are meant for a single,
specific use case:
const std::string MyClass::foo() const {
return (everything == OK) ? member_string : EmptyString();
}
Here you cannot return string(), because it's destroyed before the
function returns, and the caller receives garbage; and you don't want to
have the function return by value, because you can access the member
variable directly and save a copy. The utility functions give you a
global
empty string that you can safely return a const reference to.
DON'T USE THESE OUTSIDE THIS CASE. You should never use these as
initializers, arguments to functions, or return values in functions that
return by value. Just use the default constructor; that's what it's
there
for.
Out of curiosity, what is wrong with using EmptyString() in those
cases? Is there a correctness problem? Unnecessary inclusion of
string_util.h?
probably just a tad more costly since it involves SingletonT. but, i
think peter's main reason was simply to stick to the simpler and more
familiar std::string.
-darin
Where as It looks like GURL::EmptyGURL() may be a tad less costly than
GURL().
- a
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
On Thu, Jan 7, 2010 at 8:00 PM, Michael Nordman micha...@google.com wrote: Where as It looks like GURL::EmptyGURL() may be a tad less costly than GURL(). Not if you ever need to initialize another GURL with it (since the compiler can't collapse the copy). Which is true much of the time that EmptyGURL() can be replaced by GURL(). And the code clarity reasons still stand. Please don't do this in the name of hoping you'll save an instruction somewhere, it's much like premature optimization. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
