Cleanup: Initialize GURL by moving std::string

url/gurl.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef URL_GURL_H_
 #define URL_GURL_H_
 
 #include <iosfwd>
 #include <string>
 
@@ skipping 28 matching lines @@
   explicit GURL(const std::string& url_string /*, output_param_encoding*/);
   explicit GURL(const base::string16& url_string /*, output_param_encoding*/);
 
   // Constructor for URLs that have already been parsed and canonicalized. This
   // is used for conversions from KURL, for example. The caller must supply all
   // information associated with the URL, which must be correct and consistent.
   GURL(const char* canonical_spec,
        size_t canonical_spec_len,
        const url::Parsed& parsed,
        bool is_valid);
-  // Notice that we take the canonical_spec by value so that we can convert
-  // from WebURL without copying the string. When we call this constructor
-  // we pass in a temporary std::string, which lets the compiler skip the
-  // copy and just move the std::string into the function argument. In the
-  // implementation, we use swap to move the data into the GURL itself,
-  // which means we end up with zero copies.
-  GURL(std::string canonical_spec, const url::Parsed& parsed, bool is_valid);
+
+  // Notice that we can take the canonical_spec by rvalue reference so that we
+  // can convert from WebURL without copying the string. When we call this

[ki.stfu, 2015/09/21 05:55:55]

(the original message is here:
https://codereview.chromium.org/1349783006/diff/20001/url/gurl.h#newcode51)

Please take into account the 2nd version which uses movable arguments. In this
case, compiler moves an object and there is no copying of the string.
Initially I wanted to do only the version with movable std::string, but the 1st
is required for unittests.

Thus, it does not reduce the performance, but on the contrary, it causes the
compiler to move the object.

+  // constructor we pass in a temporary std::string, which lets the compiler
+  // skip the copy and just move the std::string into the function argument.
+  // The const reference version is used in unittests.
+  GURL(const std::string& canonical_spec,
+       const url::Parsed& parsed,
+       bool is_valid);
+  GURL(std::string&& canonical_spec, const url::Parsed& parsed,

[danakj, 2016/04/29 02:17:54]

According to the google style guide the way to write this is "std::string", and
when you std::move() at the callsite, it will move the contents into this string
here.

Moving a string is pretty cheap, I'd expect that trying to avoid an extra move
here is not buying you much?

Do you have performance numbers justifying this?

[ki.stfu, 2016/05/25 16:53:33]

I can't find a mention about this way in style guide.
Right.
No, I didn't measure it in practice. Just theoretical reasoning.

[danakj, 2016/05/25 19:59:37]

https://google.github.io/styleguide/cppguide.html#Rvalue_references

"Use rvalue references only to define move constructors and move assignment
operators, or for perfect forwarding."

i.e. Don't use it for passing to methods.

[ki.stfu, 2016/05/26 05:15:30]

Doesn't it look like a moving "for perfect forwarding"?

+       bool is_valid) noexcept;
 
   ~GURL();
 
   GURL& operator=(GURL other);
 
   // Returns true when this object represents a valid parsed URL. When not
   // valid, other functions will still succeed, but you will not get canonical
   // data out in the format you may be expecting. Instead, we keep something
   // "reasonable looking" so that the user can see how it's busted if
   // displayed to them.
@@ skipping 313 matching lines @@
   // Used for nested schemes [currently only filesystem:].
   scoped_ptr<GURL> inner_url_;
 
   // TODO bug 684583: Add encoding for query params.
 };
 
 // Stream operator so GURL can be used in assertion statements.
 URL_EXPORT std::ostream& operator<<(std::ostream& out, const GURL& url);
 
 #endif  // URL_GURL_H_