Use v8's utf8 <-> wide conversion for Linux sys_string_conversion.

Use v8's utf8 <-> wide conversion for Linux sys_string_conversion.

BUG=14826

[Dean McNamee, 2009-07-13 10:56:57]

After an hour of reading details of iconv(), I decided this was easier.

[Dean McNamee, 2009-07-14 14:20:03]

ping

On 2009/07/13 10:56:57, Dean McNamee wrote:
> After an hour of reading details of iconv(), I decided this was easier.

[Evan Martin, 2009-07-14 15:21:28]

It seems a bit silly to have this same code repeated from ICU.  Maybe we could
ask Jungshik or someone who understands ICU to implement it that way?

[Evan Martin, 2009-07-14 15:26:16]

Dean is kind for being patient for one as slow as me

LGTM

[darin (slow to review), 2009-07-14 16:51:40]

sorry for the drive-by...

http://codereview.chromium.org/149526/diff/1/2
File base/sys_string_conversions_linux.cc (right):

http://codereview.chromium.org/149526/diff/1/2#newcode25
Line 25: size_t WriteUnicodeCharAsUTF8(char* str, uint32_t c) {
this stuff seems like it would be better broken out as a base/utf8_util.{h,cc}
file.

also, why not just use iconv here, if you need to avoid ICU--do you??  why write
all of this code?

[Dean McNamee, 2009-07-14 17:15:30]

The code was already written, and well tested (it is from V8, and
passes a ton of unicode related tests), and it is simpler than using
iconv.

On Tue, Jul 14, 2009 at 6:51 PM, <darin@chromium.org> wrote:
> sorry for the drive-by...
>
>
> http://codereview.chromium.org/149526/diff/1/2
> File base/sys_string_conversions_linux.cc (right):
>
> http://codereview.chromium.org/149526/diff/1/2#newcode25
> Line 25: size_t WriteUnicodeCharAsUTF8(char* str, uint32_t c) {
> this stuff seems like it would be better broken out as a
> base/utf8_util.{h,cc} file.
>
> also, why not just use iconv here, if you need to avoid ICU--do you??
> why write all of this code?
>
> http://codereview.chromium.org/149526
>

[darin (slow to review), 2009-07-14 17:30:22]

Hmm... sys_string_conversions.h answers my question about ICU.
-Darin


On Tue, Jul 14, 2009 at 10:29 AM, Darin Fisher <darin@chromium.org> wrote:

> I'm sure it is well written and well tested, but it is not a trivial chunk
> of code, and code duplication sucks.  Think of someone maintaining this code
> in the future.  I agree the code for using iconv is not trivial, but it is
> also not nearly as subtle.
> I'm just really surprised to see code duplication like this.
>
> What about my other questions?  Why bury this in this source file and why
> not use ICU?  Shouldn't there be unit tests for these individual methods?
>  (Just because the code comes from a well tested source, doesn't mean this
> code is off-the-hook for testing.  It needs tests of its own in case someone
> wants to change it in the future.)
>
> -Darin
>
>
>
> On Tue, Jul 14, 2009 at 10:13 AM, Dean McNamee <deanm@chromium.org> wrote:
>
>> The code was already written, and well tested (it is from V8, and
>> passes a ton of unicode related tests), and it is simpler than using
>> iconv.
>>
>> On Tue, Jul 14, 2009 at 6:51 PM, <darin@chromium.org> wrote:
>> > sorry for the drive-by...
>> >
>> >
>> > http://codereview.chromium.org/149526/diff/1/2
>> > File base/sys_string_conversions_linux.cc (right):
>> >
>> > http://codereview.chromium.org/149526/diff/1/2#newcode25
>> > Line 25: size_t WriteUnicodeCharAsUTF8(char* str, uint32_t c) {
>> > this stuff seems like it would be better broken out as a
>> > base/utf8_util.{h,cc} file.
>> >
>> > also, why not just use iconv here, if you need to avoid ICU--do you??
>> > why write all of this code?
>> >
>> > http://codereview.chromium.org/149526
>> >
>>
>
>

[Dean McNamee, 2009-07-14 17:31:43]

Look at the code that already exists for using the system APIs.  I
could write something like that to use iconv, but it's even more
confusing that just using the UTF-8 code.

Btw, the UTF-8 is not that difficult, it is pretty straightforward if
you just read it for a minute.

On Tue, Jul 14, 2009 at 7:29 PM, Darin Fisher<darin@chromium.org> wrote:
> Hmm...=A0sys_string_conversions.h answers my question about ICU.
> -Darin
>
> On Tue, Jul 14, 2009 at 10:29 AM, Darin Fisher <darin@chromium.org> wrote=
:
>>
>> I'm sure it is well written and well tested, but it is not a trivial chu=
nk
>> of code, and code duplication sucks. =A0Think of someone maintaining thi=
s code
>> in the future. =A0I agree the code for using iconv is not trivial, but i=
t is
>> also not nearly as subtle.
>> I'm just really surprised to see code duplication like this.
>> What about my other questions? =A0Why bury this in this source file and =
why
>> not use ICU? =A0Shouldn't there be unit tests for these individual metho=
ds?
>> =A0(Just because the code comes from a well tested source, doesn't mean =
this
>> code is off-the-hook for testing. =A0It needs tests of its own in case s=
omeone
>> wants to change it in the future.)
>> -Darin
>>
>>
>> On Tue, Jul 14, 2009 at 10:13 AM, Dean McNamee <deanm@chromium.org> wrot=
e:
>>>
>>> The code was already written, and well tested (it is from V8, and
>>> passes a ton of unicode related tests), and it is simpler than using
>>> iconv.
>>>
>>> On Tue, Jul 14, 2009 at 6:51 PM, <darin@chromium.org> wrote:
>>> > sorry for the drive-by...
>>> >
>>> >
>>> > http://codereview.chromium.org/149526/diff/1/2
>>> > File base/sys_string_conversions_linux.cc (right):
>>> >
>>> > http://codereview.chromium.org/149526/diff/1/2#newcode25
>>> > Line 25: size_t WriteUnicodeCharAsUTF8(char* str, uint32_t c) {
>>> > this stuff seems like it would be better broken out as a
>>> > base/utf8_util.{h,cc} file.
>>> >
>>> > also, why not just use iconv here, if you need to avoid ICU--do you??
>>> > why write all of this code?
>>> >
>>> > http://codereview.chromium.org/149526
>>> >
>>
>
>

[darin (slow to review), 2009-07-14 17:47:57]

I'm sure it is well written and well tested, but it is not a trivial chunk
of code, and code duplication sucks.  Think of someone maintaining this code
in the future.  I agree the code for using iconv is not trivial, but it is
also not nearly as subtle.
I'm just really surprised to see code duplication like this.

What about my other questions?  Why bury this in this source file and why
not use ICU?  Shouldn't there be unit tests for these individual methods?
 (Just because the code comes from a well tested source, doesn't mean this
code is off-the-hook for testing.  It needs tests of its own in case someone
wants to change it in the future.)

-Darin



On Tue, Jul 14, 2009 at 10:13 AM, Dean McNamee <deanm@chromium.org> wrote:

> The code was already written, and well tested (it is from V8, and
> passes a ton of unicode related tests), and it is simpler than using
> iconv.
>
> On Tue, Jul 14, 2009 at 6:51 PM, <darin@chromium.org> wrote:
> > sorry for the drive-by...
> >
> >
> > http://codereview.chromium.org/149526/diff/1/2
> > File base/sys_string_conversions_linux.cc (right):
> >
> > http://codereview.chromium.org/149526/diff/1/2#newcode25
> > Line 25: size_t WriteUnicodeCharAsUTF8(char* str, uint32_t c) {
> > this stuff seems like it would be better broken out as a
> > base/utf8_util.{h,cc} file.
> >
> > also, why not just use iconv here, if you need to avoid ICU--do you??
> > why write all of this code?
> >
> > http://codereview.chromium.org/149526
> >
>

[Evan Martin, 2010-04-19 16:52:44]

Did we conclude what to do with this?  (Cleaning out my review queue.)