Change UTF8ToUTF16 to accept const StringPiece&.

Change UTF8ToUTF16 to accept const StringPiece&.

BUG=70936
TEST=All unit tests should pass.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72921

[Avi (use Gerrit), 2011-01-26 21:58:55]

http://codereview.chromium.org/6317016/diff/1/base/utf_string_conversions.h
File base/utf_string_conversions.h (right):

http://codereview.chromium.org/6317016/diff/1/base/utf_string_conversions.h#n...
base/utf_string_conversions.h:12: #include "base/string_piece.h"
Why? We don't need it for the declaration in this file. Anyone who includes this
file and uses UTF8ToUTF16 should include it there. See the comment starting on
line 51 in this file.

[James Su, 2011-01-26 22:13:30]

http://codereview.chromium.org/6317016/diff/1/base/utf_string_conversions.h
File base/utf_string_conversions.h (right):

http://codereview.chromium.org/6317016/diff/1/base/utf_string_conversions.h#n...
base/utf_string_conversions.h:12: #include "base/string_piece.h"
On 2011/01/26 21:58:55, Avi wrote:
> Why? We don't need it for the declaration in this file. Anyone who includes
this
> file and uses UTF8ToUTF16 should include it there. See the comment starting on
> line 51 in this file.

But from string_piece.h:

// Functions or methods may use const StringPiece& parameters to accept either
// a "const char*" or a "string" value that will be implicitly converted to
// a StringPiece.  The implicit conversion means that it is often appropriate
// to include this .h file in other files rather than forward-declaring
// StringPiece as would be appropriate for most other Google classes.


And I think the users of UTF8ToUTF16() shouldn't care about the existence of
StringPiece at all, it would be strange for them to include string_piece.h in
order to use UTF8ToUTF16.

[Avi (use Gerrit), 2011-01-26 22:20:06]

I'm not convinced that including that file would help the compiler do implicit
conversions. In any case, including that file means that the comment at line 51
is a lie and you need to fix it and the function declarations below it.

[James Su, 2011-01-26 22:33:44]

On 2011/01/26 22:20:06, Avi wrote:
> I'm not convinced that including that file would help the compiler do implicit
> conversions. In any case, including that file means that the comment at line
51
> is a lie and you need to fix it and the function declarations below it.

I'd prefer to fix those functions to use StringPiece as well. And I think we can
fix string_piece.h to avoid including <algorithm>. What do you think?

[Avi (use Gerrit), 2011-01-26 22:38:42]

On 2011/01/26 22:33:44, James Su wrote:
> I'd prefer to fix those functions to use StringPiece as well.

That's what I meant by "fix the function declarations below it".

> And I think we can
> fix string_piece.h to avoid including <algorithm>. What do you think?

If you could that'd be great for everyone.

[James Su, 2011-01-26 22:41:54]

On 2011/01/26 22:33:44, James Su wrote:
> On 2011/01/26 22:20:06, Avi wrote:
> > I'm not convinced that including that file would help the compiler do
implicit
> > conversions. In any case, including that file means that the comment at line
> 51
> > is a lie and you need to fix it and the function declarations below it.
> 
> I'd prefer to fix those functions to use StringPiece as well. And I think we
can
> fix string_piece.h to avoid including <algorithm>. What do you think?

Turns out that string_piece.h has already been included by some other popular
header files, especially string_util.h.

[James Su, 2011-01-27 00:25:42]

Please review again.

[Avi (use Gerrit), 2011-01-27 00:29:27]

Like. LGTM

[Ilya Sherman, 2011-01-27 01:17:48]

Since you're touching base, please wait for Brett to take a look before
committing.

[James Su, 2011-01-27 01:19:17]

On 2011/01/27 01:17:48, Ilya Sherman wrote:
> Since you're touching base, please wait for Brett to take a look before
> committing.

Ok. I'm still fixing build issues on Win bot.

[James Su, 2011-01-27 03:41:08]

Weird that I always get following link error on Win bot. Do you know what's
wrong?

ipc_tests errors:
1: base.lib(values.obj) : error LNK2019: unresolved external symbol "class
std::basic_string<wchar_t,struct std::char_traits<wchar_t>,class
std::allocator<wchar_t> > __cdecl UTF8ToUTF16(class base::StringPiece const &)"
(?UTF8ToUTF16@@YA?AV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@ABVStringPiece@base@@@Z)
referenced in function "public: virtual bool __thiscall
StringValue::GetAsString(class std::basic_string<wchar_t,struct
std::char_traits<wchar_t>,class std::allocator<wchar_t> > *)const "
(?GetAsString@StringValue@@UBE_NPAV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@Z)



2: base.lib(json_writer.obj) : error LNK2001: unresolved external symbol "class
std::basic_string<wchar_t,struct std::char_traits<wchar_t>,class
std::allocator<wchar_t> > __cdecl UTF8ToUTF16(class base::StringPiece const &)"
(?UTF8ToUTF16@@YA?AV?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@ABVStringPiece@base@@@Z)



3: e:\b\build\slave\win\build\src\build\Debug\ipc_tests.exe : fatal error
LNK1120: 1 unresolved externals

[brettw, 2011-01-27 04:10:41]

Thanks for making the include files have fewer deps.

http://codereview.chromium.org/6317016/diff/29001/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/6317016/diff/29001/base/logging.h#newcode927
base/logging.h:927: extern std::ostream& operator<<(std::ostream& o, const
StringPiece& piece);
What's the advantage of moving this? We were just discussing wanting to remove
the wstring operator<< from logging.h.

[James Su, 2011-01-27 04:19:56]

http://codereview.chromium.org/6317016/diff/29001/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/6317016/diff/29001/base/logging.h#newcode927
base/logging.h:927: extern std::ostream& operator<<(std::ostream& o, const
StringPiece& piece);
On 2011/01/27 04:10:42, brettw wrote:
> What's the advantage of moving this? We were just discussing wanting to remove
> the wstring operator<< from logging.h.

By moving this piece of code here, we can remove #include <iosfwd> from
string_piece.h.
This change shouldn't affect normal code at all, as stream should only be used
for logging.

[brettw, 2011-01-27 04:26:13]

http://codereview.chromium.org/6317016/diff/29001/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/6317016/diff/29001/base/logging.h#newcode927
base/logging.h:927: extern std::ostream& operator<<(std::ostream& o, const
StringPiece& piece);
I'm on the fence. I don't really like adding this stuff to logging.h, but
avoiding the extra include can be nice. I'd actually prefer to remove this
altogether if it's not an unreasonable amount of work.

What do other reviewers think?

[James Su, 2011-01-27 04:42:04]

http://codereview.chromium.org/6317016/diff/29001/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/6317016/diff/29001/base/logging.h#newcode927
base/logging.h:927: extern std::ostream& operator<<(std::ostream& o, const
StringPiece& piece);
On 2011/01/27 04:26:14, brettw wrote:
> I'm on the fence. I don't really like adding this stuff to logging.h, but
> avoiding the extra include can be nice. I'd actually prefer to remove this
> altogether if it's not an unreasonable amount of work.
> 
> What do other reviewers think?

Do you mean we should avoid using it even for logging? I'll try to remove it and
see how many files will be affected.

[James Su, 2011-01-27 04:59:41]

On 2011/01/27 04:42:04, James Su wrote:
> http://codereview.chromium.org/6317016/diff/29001/base/logging.h
> File base/logging.h (right):
> 
> http://codereview.chromium.org/6317016/diff/29001/base/logging.h#newcode927
> base/logging.h:927: extern std::ostream& operator<<(std::ostream& o, const
> StringPiece& piece);
> On 2011/01/27 04:26:14, brettw wrote:
> > I'm on the fence. I don't really like adding this stuff to logging.h, but
> > avoiding the extra include can be nice. I'd actually prefer to remove this
> > altogether if it's not an unreasonable amount of work.
> > 
> > What do other reviewers think?
> 
> Do you mean we should avoid using it even for logging? I'll try to remove it
and
> see how many files will be affected.

Quite a lot files are using it, including logging.cc itself. So maybe it's
better to keep as it for now.

[James Su, 2011-01-27 19:32:21]

Ping.

On 2011/01/27 04:59:41, James Su wrote:
> On 2011/01/27 04:42:04, James Su wrote:
> > http://codereview.chromium.org/6317016/diff/29001/base/logging.h
> > File base/logging.h (right):
> > 
> > http://codereview.chromium.org/6317016/diff/29001/base/logging.h#newcode927
> > base/logging.h:927: extern std::ostream& operator<<(std::ostream& o, const
> > StringPiece& piece);
> > On 2011/01/27 04:26:14, brettw wrote:
> > > I'm on the fence. I don't really like adding this stuff to logging.h, but
> > > avoiding the extra include can be nice. I'd actually prefer to remove this
> > > altogether if it's not an unreasonable amount of work.
> > > 
> > > What do other reviewers think?
> > 
> > Do you mean we should avoid using it even for logging? I'll try to remove it
> and
> > see how many files will be affected.
> 
> Quite a lot files are using it, including logging.cc itself. So maybe it's
> better to keep as it for now.

[brettw, 2011-01-28 00:43:44]

Okay, you convinced me. LGTM