Fix a 'type-limits' warning in StringSlicer::FindValidBoundaryAfter

Fix a 'type-limits' warning in StringSlicer::FindValidBoundaryAfter

StringSlicer::FindValidBoundaryAfter passes a size_t value to the
'length' argument of U16_SET_CP_LIMIT, which expects the argument to be
an int32. When compiling with -Werror=type-limits, that results in the
following compilation error:

../../ui/gfx/text_elider.cc: In member function 'size_t
gfx::StringSlicer::FindValidBoundaryAfter(size_t) const':
../../ui/gfx/text_elider.cc:150:59: error: comparison of unsigned
expression < 0 is always false [-Werror=type-limits]
     U16_SET_CP_LIMIT(text_.data(), 0, index, text_.length());
                                                           ^
../../third_party/icu/source/common/unicode/utf16.h:618:41: note: in
definition of macro 'U16_SET_CP_LIMIT'
     if((start)<(i) && ((i)<(length) || (length)<0) &&
U16_IS_LEAD((s)[(i)-1]) && U16_IS_TRAIL((s)[i])) { \
                                         ^

This CL fixes the 'type-limits' warning above.

BUG=424334, 427726
TEST=Build successfully with '-Werror=type-limits' using the GN flow.

Committed: https://crrev.com/492b0cf0da08a9d102bb5465b298b797ef312068
Cr-Commit-Position: refs/heads/master@{#301935}

[Ben Chan, 2014-10-29 00:27:12]

benchan@chromium.org changed reviewers:
+ msw@chromium.org

[Ben Chan, 2014-10-29 00:32:37]

Disabling type-limits check would be too aggressive, so I think we could either
do static_cast<int>(text_.length()) or, like this patch, suppress the warning
via pragma. Not sure if there is a better way to handle this, but I'm open for
other solutions.

[Ben Chan, 2014-10-29 16:51:15]

benchan@chromium.org changed reviewers:
+ danakj@chromium.org, oshima@chromium.org

[Ben Chan, 2014-10-29 16:52:00]

adding a few more reviewers as I'm not sure who's the main reviewer for this
class

[msw, 2014-10-29 16:54:57]

msw@chromium.org changed reviewers:
+ pkasting@chromium.org

[msw, 2014-10-29 16:54:58]

Peter is working on similar issues, so he may have some advice. I suspect the
right fix involves accepting or passing a different type, not suppressing the
warning like this.

[Peter Kasting, 2014-10-29 17:08:00]

https://codereview.chromium.org/681123006/diff/1/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/1/ui/gfx/text_elider.cc#newcod...
ui/gfx/text_elider.cc:151: #endif
Definitely don't fix the warning this way -- the right answer is to change the
code so it doesn't produce the warning.

It's somewhat unfortunate that ICU defines all these methods as macros instead
of as functions.  I wonder if they'd ever be willing to change that.  We have
some ICU contributors on the team, you might ask them.

In the meantime, there are a couple obvious possible fixes here:

    U16_SET_CP_LIMIT(text_.c_str(), 0, index, -1);

or

    U16_SET_CP_LIMIT(text_.data(), 0, index,
                     base::checked_cast<int32_t>(text_.length()));

I'd probably do the former since it's simplest.

[Ben Chan, 2014-10-29 17:21:11]

On 2014/10/29 17:08:00, Peter Kasting wrote:
> https://codereview.chromium.org/681123006/diff/1/ui/gfx/text_elider.cc
> File ui/gfx/text_elider.cc (right):
> 
>
https://codereview.chromium.org/681123006/diff/1/ui/gfx/text_elider.cc#newcod...
> ui/gfx/text_elider.cc:151: #endif
> Definitely don't fix the warning this way -- the right answer is to change the
> code so it doesn't produce the warning.
> 
> It's somewhat unfortunate that ICU defines all these methods as macros instead
> of as functions.  I wonder if they'd ever be willing to change that.  We have
> some ICU contributors on the team, you might ask them.
> 
> In the meantime, there are a couple obvious possible fixes here:
> 
>     U16_SET_CP_LIMIT(text_.c_str(), 0, index, -1);
> 
> or
> 
>     U16_SET_CP_LIMIT(text_.data(), 0, index,
>                      base::checked_cast<int32_t>(text_.length()));
> 
> I'd probably do the former since it's simplest.

Thanks for the suggestion. PTAL

[Peter Kasting, 2014-10-29 17:33:38]

LGTM with comment

https://codereview.chromium.org/681123006/diff/20001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/20001/ui/gfx/text_elider.cc#ne...
ui/gfx/text_elider.cc:150: U16_SET_CP_LIMIT(text_.data(), 0, index, -1);
For safety, this should use c_str() instead of data(), as the former is
guaranteed to be null-terminated and the latter is not.  Passing a negative
length is supposedly only legal with a null-terminated string.

(The macro here doesn't end up caring about such a thing, but it might in the
future.)

[Ben Chan, 2014-10-29 17:36:01]

https://codereview.chromium.org/681123006/diff/20001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/20001/ui/gfx/text_elider.cc#ne...
ui/gfx/text_elider.cc:150: U16_SET_CP_LIMIT(text_.data(), 0, index, -1);
On 2014/10/29 17:33:38, Peter Kasting wrote:
> For safety, this should use c_str() instead of data(), as the former is
> guaranteed to be null-terminated and the latter is not.  Passing a negative
> length is supposedly only legal with a null-terminated string.
> 
> (The macro here doesn't end up caring about such a thing, but it might in the
> future.)

Done.

[Ben Chan, 2014-10-29 17:38:48]

The CQ bit was checked by benchan@chromium.org

[commit-bot: I haz the power, 2014-10-29 17:39:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/681123006/40001

[commit-bot: I haz the power, 2014-10-29 17:48:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 17:48:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Chan, 2014-10-29 18:02:22]

On 2014/10/29 17:48:35, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_dbg on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
>   android_dbg_tests_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
>   linux_chromium_gn_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

seems like we need to cast |index| to int32 as well, or we will end up a
sign-compare warning :(

[Peter Kasting, 2014-10-29 18:03:31]

On 2014/10/29 18:02:22, Ben Chan wrote:
> On 2014/10/29 17:48:35, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_chromium_gn_compile_dbg on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
> >   android_dbg_tests_recipe on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> >   linux_chromium_gn_rel on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> seems like we need to cast |index| to int32 as well, or we will end up a
> sign-compare warning :(

Makes sense.  I'd probably go ahead and checked_cast it rather than static_cast,
just for safety.

[Ben Chan, 2014-10-29 18:08:23]

Patchset #4 (id:60001) has been deleted

[Ben Chan, 2014-10-29 18:17:36]

On 2014/10/29 18:03:31, Peter Kasting wrote:
> On 2014/10/29 18:02:22, Ben Chan wrote:
> > On 2014/10/29 17:48:35, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   android_chromium_gn_compile_dbg on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
> > >   android_dbg_tests_recipe on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> > >   linux_chromium_gn_rel on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > seems like we need to cast |index| to int32 as well, or we will end up a
> > sign-compare warning :(
> 
> Makes sense.  I'd probably go ahead and checked_cast it rather than
static_cast,
> just for safety.

Done. I need to assign |index| to a int32 variable for ++(i) in the macro to
work

[Peter Kasting, 2014-10-29 18:30:04]

Still LGTM

[Ben Chan, 2014-10-29 19:13:23]

On 2014/10/29 18:30:04, Peter Kasting wrote:
> Still LGTM

sorry, should have realized the side-effect of ++index. fixed in patch set 5 :(

[Peter Kasting, 2014-10-29 19:17:26]

I hang my head for having signed off on the previous version.  Sloppy.

https://codereview.chromium.org/681123006/diff/100001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/100001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:154: index = base::checked_cast<size_t>(text_index);
How about this:

  if (index == text_.length())
    return index;
  int32_t text_index = ...
  ...
  return static_cast<size_t>(text_index);

It should be OK to use a static_cast in the return statement since
text_.length() successfully checked_casted to an int32_t.  And this
restructuring of the code makes it harder for someone in the future to break the
code by not returning the updated value.

[Ben Chan, 2014-10-29 19:23:24]

On 2014/10/29 19:17:26, Peter Kasting wrote:
> I hang my head for having signed off on the previous version.  Sloppy.
> 
> https://codereview.chromium.org/681123006/diff/100001/ui/gfx/text_elider.cc
> File ui/gfx/text_elider.cc (right):
> 
>
https://codereview.chromium.org/681123006/diff/100001/ui/gfx/text_elider.cc#n...
> ui/gfx/text_elider.cc:154: index = base::checked_cast<size_t>(text_index);
> How about this:
> 
>   if (index == text_.length())
>     return index;
>   int32_t text_index = ...
>   ...
>   return static_cast<size_t>(text_index);
> 
> It should be OK to use a static_cast in the return statement since
> text_.length() successfully checked_casted to an int32_t.  And this
> restructuring of the code makes it harder for someone in the future to break
the
> code by not returning the updated value.

sgtm. thanks

[Ben Chan, 2014-10-29 19:59:36]

On 2014/10/29 19:23:24, Ben Chan wrote:
> On 2014/10/29 19:17:26, Peter Kasting wrote:
> > I hang my head for having signed off on the previous version.  Sloppy.
> > 
> > https://codereview.chromium.org/681123006/diff/100001/ui/gfx/text_elider.cc
> > File ui/gfx/text_elider.cc (right):
> > 
> >
>
https://codereview.chromium.org/681123006/diff/100001/ui/gfx/text_elider.cc#n...
> > ui/gfx/text_elider.cc:154: index = base::checked_cast<size_t>(text_index);
> > How about this:
> > 
> >   if (index == text_.length())
> >     return index;
> >   int32_t text_index = ...
> >   ...
> >   return static_cast<size_t>(text_index);
> > 
> > It should be OK to use a static_cast in the return statement since
> > text_.length() successfully checked_casted to an int32_t.  And this
> > restructuring of the code makes it harder for someone in the future to break
> the
> > code by not returning the updated value.
> 
> sgtm. thanks

verified locally and via tryjobs. ptal.

[Peter Kasting, 2014-10-29 20:00:04]

LGTM

[Ben Chan, 2014-10-29 20:00:21]

The CQ bit was checked by benchan@chromium.org

[commit-bot: I haz the power, 2014-10-29 20:01:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/681123006/120001

[commit-bot: I haz the power, 2014-10-29 20:12:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 20:12:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ben Chan, 2014-10-29 20:26:02]

On 2014/10/29 20:12:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Can someone provide an OWNER lgtm? Thanks

[msw, 2014-10-29 20:46:44]

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:155: U16_SET_CP_LIMIT(text_.data(), 0, text_index,
text_length);
Didn't Peter suggest using c_str() instead of data()?

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:156: return static_cast<size_t>(text_index);
Shouldn't this check that the 32-bit index won't overflow as a 16-bit size_t or
do a saturated conversion or something?

[Ben Chan, 2014-10-29 21:08:30]

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:155: U16_SET_CP_LIMIT(text_.data(), 0, text_index,
text_length);
On 2014/10/29 20:46:43, msw wrote:
> Didn't Peter suggest using c_str() instead of data()?

I believe .c_str() isn't necessary if the length is given.

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:156: return static_cast<size_t>(text_index);
On 2014/10/29 20:46:44, msw wrote:
> Shouldn't this check that the 32-bit index won't overflow as a 16-bit size_t
or
> do a saturated conversion or something?

do we support a platform with 16-bit size_t? if so, 32-bit index/length doesn't
feel right either.

[msw, 2014-10-29 21:15:46]

lgtm

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:155: U16_SET_CP_LIMIT(text_.data(), 0, text_index,
text_length);
On 2014/10/29 21:08:30, Ben Chan wrote:
> On 2014/10/29 20:46:43, msw wrote:
> > Didn't Peter suggest using c_str() instead of data()?
> 
> I believe .c_str() isn't necessary if the length is given.

If c_str() offers additional safety without serious negative implications, let's
use that; otherwise, I it's fine to continue using data() here.

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:156: return static_cast<size_t>(text_index);
On 2014/10/29 21:08:30, Ben Chan wrote:
> On 2014/10/29 20:46:44, msw wrote:
> > Shouldn't this check that the 32-bit index won't overflow as a 16-bit size_t
> or
> > do a saturated conversion or something?
> 
> do we support a platform with 16-bit size_t? if so, 32-bit index/length
doesn't
> feel right either.

Heh, perhaps not, I guess this ought to be fine otherwise.

[Ben Chan, 2014-10-29 21:23:37]

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/681123006/diff/120001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:155: U16_SET_CP_LIMIT(text_.data(), 0, text_index,
text_length);
On 2014/10/29 21:15:46, msw wrote:
> On 2014/10/29 21:08:30, Ben Chan wrote:
> > On 2014/10/29 20:46:43, msw wrote:
> > > Didn't Peter suggest using c_str() instead of data()?
> > 
> > I believe .c_str() isn't necessary if the length is given.
> 
> If c_str() offers additional safety without serious negative implications,
let's
> use that; otherwise, I it's fine to continue using data() here.

The only thing I can think of may be related to performance, depending on the
implementation. It won't be an issue after we move to C++11, which requires both
.data() and .c_str() to return null-terminated buffer.

[Ben Chan, 2014-10-29 21:23:42]

The CQ bit was checked by benchan@chromium.org

[commit-bot: I haz the power, 2014-10-29 21:27:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/681123006/120001

[commit-bot: I haz the power, 2014-10-29 21:33:35]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-10-29 21:34:36]

Patchset 6 (id:??) landed as
https://crrev.com/492b0cf0da08a9d102bb5465b298b797ef312068
Cr-Commit-Position: refs/heads/master@{#301935}