base: Move SplitStringDontTrim functions from string_util.h to string_split.h

base: Move SplitStringDontTrim functions from string_util.h to string_split.h

BUG=None
TEST=trybots

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

[tfarina, 2010-09-08 18:49:00]

ping!

[jungshik at Google, 2010-09-08 20:04:06]

http://codereview.chromium.org/3366011/diff/5001/6002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/5001/6002#newcode110
base/string_split.cc:110: const typename STR::value_type s,
This does not work well for string16 (and wstring on Windows) because its
value_type (char16) does not always represent a character.  It can be a
first/second half of a surrogate pair. 

We need to audit all string16 functions that use 'char16'...

It's even worse for UTF-8 because 'char' can be a part of a multi-byte sequence
representing a single character.

http://codereview.chromium.org/3366011/diff/5001/6002#newcode141
base/string_split.cc:141: std::vector<string16>* r) {
Please, add check to see if 's' is within BMP.  You can use
!CBU16_IS_SURROGATE(c) in base/third_party/icu_utf.h

http://codereview.chromium.org/3366011/diff/5001/6002#newcode148
base/string_split.cc:148: std::vector<std::string>* r) {
Add a check to make sure that 's' is in the ASCII range.

http://codereview.chromium.org/3366011/diff/5001/6003
File base/string_split.h (right):

http://codereview.chromium.org/3366011/diff/5001/6003#newcode45
base/string_split.h:45: std::vector<std::string>* r);
Along with (D)CHECKs in the implementation, please document that |s| must be in
the ASCII range or in BMP (Basic Multilingual Plane) for string16 (and wstring
version on Windows).

[jungshik at Google, 2010-09-08 20:07:12]

http://codereview.chromium.org/3366011/diff/5001/6002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/5001/6002#newcode141
base/string_split.cc:141: std::vector<string16>* r) {
On 2010/09/08 20:04:06, Jungshik Shin wrote:
> Please, add check to see if 's' is within BMP.  You can use
> !CBU16_IS_SURROGATE(c) in base/third_party/icu_utf.h 


oops. I meant " CBU16_IS_SINGLE(s)"

[jungshik at Google, 2010-09-08 20:10:02]

http://codereview.chromium.org/3366011/diff/5001/6002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/5001/6002#newcode134
base/string_split.cc:134: std::vector<std::wstring>* r) {
Where WCHAR_T_IS_UTF16 is true, 

you also need DCHECK for CBU16_IS_SINGLE(s) here.

http://codereview.chromium.org/3366011/diff/5001/6006
File base/string_util.h (right):

http://codereview.chromium.org/3366011/diff/5001/6006#newcode524
base/string_util.h:524: std::vector<std::string>* r);
Why aren't you moving these as well? 

BTW, the same DCHECK and the comment about |s| have to be added to all the
SplitString. 

And, I guess 'c' is better than 's' as the argument name.

[tfarina, 2010-09-09 01:01:26]

http://codereview.chromium.org/3366011/diff/5001/6002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/5001/6002#newcode134
base/string_split.cc:134: std::vector<std::wstring>* r) {
On 2010/09/08 20:10:02, Jungshik Shin wrote:
> Where WCHAR_T_IS_UTF16 is true, 
> 
> you also need DCHECK for CBU16_IS_SINGLE(s) here.

Done.

http://codereview.chromium.org/3366011/diff/5001/6002#newcode141
base/string_split.cc:141: std::vector<string16>* r) {
On 2010/09/08 20:07:12, Jungshik Shin wrote:
> On 2010/09/08 20:04:06, Jungshik Shin wrote:
> > Please, add check to see if 's' is within BMP.  You can use
> > !CBU16_IS_SURROGATE(c) in base/third_party/icu_utf.h 
> 
> 
> oops. I meant " CBU16_IS_SINGLE(s)"  
> 
> 

Done.

http://codereview.chromium.org/3366011/diff/5001/6002#newcode148
base/string_split.cc:148: std::vector<std::string>* r) {
On 2010/09/08 20:04:06, Jungshik Shin wrote:
> Add a check to make sure that 's' is in the ASCII range.

Done.

http://codereview.chromium.org/3366011/diff/5001/6003
File base/string_split.h (right):

http://codereview.chromium.org/3366011/diff/5001/6003#newcode45
base/string_split.h:45: std::vector<std::string>* r);
On 2010/09/08 20:04:06, Jungshik Shin wrote:
> Along with (D)CHECKs in the implementation, please document that |s| must be
in
> the ASCII range or in BMP (Basic Multilingual Plane) for string16 (and wstring
> version on Windows). 
> 

Done.

http://codereview.chromium.org/3366011/diff/5001/6006
File base/string_util.h (right):

http://codereview.chromium.org/3366011/diff/5001/6006#newcode524
base/string_util.h:524: std::vector<std::string>* r);
On 2010/09/08 20:10:02, Jungshik Shin wrote:
> Why aren't you moving these as well? 
> 
Because it will make the patch much bigger :(

[tfarina, 2010-09-09 02:02:58]

I'm really intrigued why this is failing to compile on the trybots but *not*
locally :(

[jungshik at Google, 2010-09-09 18:48:17]

thanks for addressing comments.

http://codereview.chromium.org/3366011/diff/20001/21002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/20001/21002#newcode137
base/string_split.cc:137: DCHECK(CBU16_IS_SINGLE(c));
This DCHECK should be :

#if defined(WCHAR_T_IS_UTF16)
DCHECK(CBU16_IS_SINGLE(c));
#else
DCHECK(CBU_IS_UNICODE_CHAR(c));
#endif

http://codereview.chromium.org/3366011/diff/20001/21002#newcode155
base/string_split.cc:155: DCHECK(IsStringASCII(ASCIIToUTF16(tmp)));
looks like we'd better add IsStringASC(const std::string&)?

[brettw, 2010-09-09 21:38:19]

http://codereview.chromium.org/3366011/diff/20001/21002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/20001/21002#newcode155
base/string_split.cc:155: DCHECK(IsStringASCII(ASCIIToUTF16(tmp)));
This entire 3-line thing can be replaced with:
  DCHECK(c < 0x80);

[jungshik at Google, 2010-09-09 22:36:03]

http://codereview.chromium.org/3366011/diff/20001/21002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/20001/21002#newcode155
base/string_split.cc:155: DCHECK(IsStringASCII(ASCIIToUTF16(tmp)));
On 2010/09/09 21:38:20, brettw wrote:
> This entire 3-line thing can be replaced with:
>   DCHECK(c < 0x80);

oops. absolutely ! I didn't even spot the two spurious lines before DCHECK.

http://codereview.chromium.org/3366011/diff/20001/21003
File base/string_split.h (right):

http://codereview.chromium.org/3366011/diff/20001/21003#newcode37
base/string_split.h:37: // NOTE: |c| must be in BMP (Basic Multilingual Plane)
Where wchar_t is char16, this is true (i.e. Windows). Elsewhere (Linux/Mac),
wchar_t should be a valid Unicode code point (32-bit).

http://codereview.chromium.org/3366011/diff/20001/21003#newcode45
base/string_split.h:45: // Note: |c| must be in the ASCII range.
One more note: |str| should not be in a multi-byte encoding like Shift-JIS or
GBK in which the trailing byte of a multi-byte character can be in the ASCII
range. UTF-8, and other single/multi-byte ASCII-compatible encodings are OK. I
guess all the current callers use UTF-8. In that case, you can document that and
add DCHECK for UTF-8ness, I think.

[tfarina, 2010-09-10 01:46:45]

http://codereview.chromium.org/3366011/diff/20001/21002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/20001/21002#newcode137
base/string_split.cc:137: DCHECK(CBU16_IS_SINGLE(c));
On 2010/09/09 18:48:17, Jungshik Shin wrote:
> This DCHECK should be :
> 
> #if defined(WCHAR_T_IS_UTF16)
> DCHECK(CBU16_IS_SINGLE(c));
> #else
> DCHECK(CBU_IS_UNICODE_CHAR(c));
> #endif
> 
This doesn't compile on linux:

CXX(target) out/Debug/obj.target/base/base/string_split.o
base/string_split.cc: In function ‘void base::SplitStringDontTrim(const
std::wstring&, wchar_t, std::vector<std::basic_string<wchar_t,
std::char_traits<wchar_t>, std::allocator<wchar_t> >,
std::allocator<std::basic_string<wchar_t, std::char_traits<wchar_t>,
std::allocator<wchar_t> > > >*)’:
base/string_split.cc:140: error: ‘U_IS_UNICODE_NONCHAR’ was not declared in this
scope

[tfarina, 2010-09-10 02:18:44]

http://codereview.chromium.org/3366011/diff/20001/21002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/20001/21002#newcode137
base/string_split.cc:137: DCHECK(CBU16_IS_SINGLE(c));
On 2010/09/09 18:48:17, Jungshik Shin wrote:
> This DCHECK should be :
> 
> #if defined(WCHAR_T_IS_UTF16)
Probably this should be:
#if !defined(WCHAR_T_IS_UTF16)
like we do below.

http://codereview.chromium.org/3366011/diff/20001/21002#newcode155
base/string_split.cc:155: DCHECK(IsStringASCII(ASCIIToUTF16(tmp)));
On 2010/09/09 21:38:20, brettw wrote:
> This entire 3-line thing can be replaced with:
>   DCHECK(c < 0x80);

Done.

http://codereview.chromium.org/3366011/diff/20001/21003
File base/string_split.h (right):

http://codereview.chromium.org/3366011/diff/20001/21003#newcode37
base/string_split.h:37: // NOTE: |c| must be in BMP (Basic Multilingual Plane)
On 2010/09/09 22:36:03, Jungshik Shin wrote:
> Where wchar_t is char16, this is true (i.e. Windows). Elsewhere (Linux/Mac),
> wchar_t should be a valid Unicode code point (32-bit). 
> 

Done.

http://codereview.chromium.org/3366011/diff/20001/21003#newcode45
base/string_split.h:45: // Note: |c| must be in the ASCII range.
On 2010/09/09 22:36:03, Jungshik Shin wrote:
> One more note: |str| should not be in a multi-byte encoding like Shift-JIS or
> GBK in which the trailing byte of a multi-byte character can be in the ASCII
> range. UTF-8, and other single/multi-byte ASCII-compatible encodings are OK. I
> guess all the current callers use UTF-8. In that case, you can document that
and
> add DCHECK for UTF-8ness, I think.

Done.

[tfarina, 2010-09-10 22:38:26]

ping Jungshik.

[tfarina, 2010-09-14 12:44:32]

ping Jungshik/Brett again.

http://codereview.chromium.org/3366011/diff/30001/31002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/30001/31002#newcode140
base/string_split.cc:140: DCHECK(CBU_IS_UNICODE_CHAR(c));
This is not used by anyone in the code base. And doesn't compile on windows.

[brettw, 2010-09-14 15:58:52]

LGTM as long as you're actually going to finish moving the other split
functions. Moving this stuff is great, I just don't want the duplicate code to
persist very long.

[tfarina, 2010-09-14 18:18:46]

On 2010/09/14 15:58:52, brettw wrote:
> LGTM
Thanks Brett.

> as long as you're actually going to finish moving the other split
> functions.
Can I move the other split functions in another patch? I wouldn't like to make a
mess with it (it's already big though).

http://codereview.chromium.org/3366011/diff/30001/31002
File base/string_split.cc (right):

http://codereview.chromium.org/3366011/diff/30001/31002#newcode137
base/string_split.cc:137: #if !defined(WCHAR_T_IS_UTF16)
Jungshik, I'm going to remove these two DCHECK (to make the trybots happier
again) since they are causing more trouble than it's worth.

[brettw, 2010-09-14 18:51:48]

On Tue, Sep 14, 2010 at 11:18 AM,  <tfarina@chromium.org> wrote:
> On 2010/09/14 15:58:52, brettw wrote:
>>
>> LGTM
>
> Thanks Brett.
>
>> as long as you're actually going to finish moving the other split
>> functions.
>
> Can I move the other split functions in another patch? I wouldn't like to
> make a
> mess with it (it's already big though).

Yes, that's what I was expecting you to do. I just wanted to make sure
you were going to actually do it :)

Brett