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 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
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 wrote:
> On Thu, Jan 7, 2010 at 3:45 PM, Aaron Boodman wrote:
>
>> On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting
>> 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 Singleton. 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 Fri, Jan 8, 2010 at 1:51 AM, Albert J. Wong (王重傑) wrote:
> On Thu, Jan 7, 2010 at 2:46 PM, Darin Fisher wrote:
>
>>
>> On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow 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 Singleton 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 Fri, Jan 8, 2010 at 2:10 AM, Albert J. Wong (王重傑) wrote:
>
> 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 Thu, Jan 7, 2010 at 3:45 PM, Aaron Boodman wrote:
> On Thu, Jan 7, 2010 at 1:28 PM, Peter Kasting 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 Singleton. 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 Thu, Jan 7, 2010 at 3:45 PM, Aaron Boodman 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 1:28 PM, Peter Kasting 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
[ 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 2:50 PM, Albert Wong (王重傑) 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
On Thu, Jan 7, 2010 at 2:53 PM, Victor Khimenko wrote:
>
> On Fri, Jan 8, 2010 at 1:51 AM, Albert J. Wong (王重傑)
> wrote:
>
>> On Thu, Jan 7, 2010 at 2:46 PM, Darin Fisher wrote:
>>
>>>
>>> On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow 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 Singleton 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:46 PM, Darin Fisher wrote:
>
>
> On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow 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 Singleton 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 1:34 PM, Jeremy Orlow 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 Singleton 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 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 1:50 PM, Jeremy Orlow 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
What about renaming the function? EmptyStringHACK() or something? On Thu, Jan 7, 2010 at 1:49 PM, Peter Kasting wrote: > On Thu, Jan 7, 2010 at 1:43 PM, Jeremy Orlow 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:43 PM, Jeremy Orlow 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:38 PM, Peter Kasting wrote:
> On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow 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
On Thu, Jan 7, 2010 at 1:34 PM, Jeremy Orlow 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
(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 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-dev] Don't use Empty[String,WString,String16,GURL]() unless you really need to
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
