PSA: GURL::Replacements now accepts a StringPiece or char*

[Matt Giuca]

I just fixed GURL::Replacements' Set*Str methods so that they take a StringPiece rather than a std::string& [1]. If you don't use the GURL API you can stop reading now.

Previously, you had to make sure that you pass a string object that outlives the Replacements object, so you couldn't pass a char* like this:

replacements.SetSchemeStr("http");

(because the char* would create a temporary std::string which would immediately go out of scope).

You had to do this:

std::string scheme("http");

replacements.SetSchemeStr(scheme);

Now that it accepts a StringPiece, you can safely pass a char* (e.g., a string literal). Or, if you need to pass a substring, you can make a StringPiece and pass that instead (avoiding a copy).

Note that you still need to ensure the std::string, char* or StringPiece remains in scope over the lifetime of the Replacements object. But you can now use a char* directly, so stop copying them into temporary string objects.

I have updated (I believe) every single caller to avoid doing the unnecessary conversion to a std::string [1]. So this is just a PSA to avoid creating new unnecessary conversions in the future.

[1] https://codereview.chromium.org/889463003

[Peter Kasting]

On Wed, Feb 4, 2015 at 11:57 PM, Matt Giuca <mgi...@chromium.org> wrote:

Note that you still need to ensure the std::string, char* or StringPiece remains in scope over the lifetime of the Replacements object. But you can now use a char* directly, so stop copying them into temporary string objects.

So, this code:

replacements.SetSchemeStr(std::string(...));

...was broken before and is still broken now?

It's super cool that we unbroke just passing a char* directly (seriously, thank you, I just ran into this stuff today and was irritated), and that probably makes all the callsites of this fairly clean, but I find myself wishing the API would just copy, so that you could safely pass in a temporary and not have it blow up in your face.  Would that be a visible perf regression?

PK

[Matt Giuca]

On Thu Feb 05 2015 at 8:34:34 PM Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Feb 4, 2015 at 11:57 PM, Matt Giuca <mgi...@chromium.org> wrote:

Note that you still need to ensure the std::string, char* or StringPiece remains in scope over the lifetime of the Replacements object. But you can now use a char* directly, so stop copying them into temporary string objects.

So, this code:

replacements.SetSchemeStr(std::string(...));

...was broken before and is still broken now?

Yes.

The main place this manifests is in SetPortStr, because you often pass the result of IntToString like this (e.g., [1]):

    const std::string new_port = base::IntToString(port);

    replacements.SetPortStr(new_port);

And you still need to explicitly store it in a variable before passing it to SetPortStr.

It's super cool that we unbroke just passing a char* directly (seriously, thank you, I just ran into this stuff today and was irritated),

Haha yes I know you did (I got a merge conflict with you) :).

 

and that probably makes all the callsites of this fairly clean, but I find myself wishing the API would just copy, so that you could safely pass in a temporary and not have it blow up in your face.  Would that be a visible perf regression?

I haven't experimented with it (and the reason I didn't change it to a string copy is that I had no idea about the perf regressions, whereas I knew the StringPiece would maintain or improve upon the existing perf characteristics).

If I were to change it to using std::string, could I just land it and see whether the perf bots complain? Or would I have to do some additional experiments?

I filed a bug for this (http://crbug.com/455944) to continue the discussion without involving all of chromium-dev.

Matt

[1] https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stream_factory_impl.cc&l=30