Cleanup: Remove vector::at() calls in SimpleMenuModel. Do CHECKs instead.

Cleanup: Remove vector::at() calls in SimpleMenuModel. Do CHECKs instead.

BUG=none
TEST=none

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

[sky, 2012-05-30 22:57:33]

On windows using the [] operator triggers a crash if out of bounds. Do we not
have a way to do that else where?

[Lei Zhang, 2012-05-30 23:14:27]

On 2012/05/30 22:57:33, sky wrote:
> On windows using the [] operator triggers a crash if out of bounds. Do we not
> have a way to do that else where?

With a trivial test program on Linux, I've found if you do:

vector<int> foo;
printf("%d\n", foo[0]);

it will segfault.

However, if you do:

vector<int> foo;
foo.push_back(1);
foo.push_back(2);
foo.pop_back();
foo.pop_back();
printf("%d\n", foo[0]);  // This will actually print "1" !
printf("%d\n", foo.at(0));  // This will throw an exception.

[Avi (use Gerrit), 2012-05-30 23:20:36]

On 2012/05/30 22:57:33, sky wrote:
> On windows using the [] operator triggers a crash if out of bounds.

The standard doesn't guarantee that; a[n] and a.at(n) are defined the same
except that at() throws when out-of-bounds; [] can do whatever it wants.

[sky, 2012-05-30 23:21:18]

Does this imply we should use at() every where?

  -Scott

On Wed, May 30, 2012 at 4:14 PM,  <thestig@chromium.org> wrote:
> On 2012/05/30 22:57:33, sky wrote:
>>
>> On windows using the [] operator triggers a crash if out of bounds. Do we
>> not
>> have a way to do that else where?
>
>
> With a trivial test program on Linux, I've found if you do:
>
> vector<int> foo;
> printf("%d\n", foo[0]);
>
> it will segfault.
>
> However, if you do:
>
> vector<int> foo;
> foo.push_back(1);
> foo.push_back(2);
> foo.pop_back();
> foo.pop_back();
> printf("%d\n", foo[0]);  // This will actually print "1" !
> printf("%d\n", foo.at(0));  // This will throw an exception.
>
> https://chromiumcodereview.appspot.com/10458045/

[Lei Zhang, 2012-05-30 23:25:22]

That's what I'm trying to do here. I was looking at bug 123036, and I noticed we
accessed items_[index].type without crashing, and then got the OOB exception
immediately after while accessing items_[index].command_id.

On 2012/05/30 23:21:18, sky wrote:
> Does this imply we should use at() every where?

[sky, 2012-05-30 23:46:56]

I wasn't referring to just this code, but all of chrome?

  -Scott

On Wed, May 30, 2012 at 4:25 PM,  <thestig@chromium.org> wrote:
> That's what I'm trying to do here. I was looking at bug 123036, and I
> noticed we
> accessed items_[index].type without crashing, and then got the OOB exception
> immediately after while accessing items_[index].command_id.
>
>
> On 2012/05/30 23:21:18, sky wrote:
>>
>> Does this imply we should use at() every where?
>
>
> https://chromiumcodereview.appspot.com/10458045/

[Lei Zhang, 2012-05-31 00:19:29]

IMO, we should. According to the internal VectorBoundsChecking wiki page, even
Valgrind does not always catch this type of bugs.

On 2012/05/30 23:46:56, sky wrote:
> I wasn't referring to just this code, but all of chrome?

[Peter Kasting, 2012-05-31 00:28:17]

Do not use at() here or elsewhere.  at() is both extremely uncommon and
informally banned in Chromium code.  I am happy to formalize that ban if
necessary.

at() explicitly bounds-checks all accesses and throws an exception when the
check fails.  Doing an explicit check is rarely what people intend or want.  In
rare cases it can have performance consequences.  And we generally frown on
exception-throwing code given that we explicitly compile out exception support.

Added to this:
  * at() is less readable -- it's both more verbose and less idiomatic.
  * Many people don't truly understand the difference between [] and at()
(witness scattered random usage of at() we've been cleaning up for years).

If you want a bounds check, check the bounds yourself; an explicit CHECK or
DCHECK is far more obvious and is preferable in the rare case that we truly want
to always bounds-check.  Or add other appropriate assertions to guarantee code
behaves as you expect.

[Lei Zhang, 2012-05-31 03:55:32]

In that case, I'll convert them to CHECKs to preserve the existing behavior. See
patch set 2.

On 2012/05/31 00:28:17, Peter Kasting wrote:
> Do not use at() here or elsewhere.  at() is both extremely uncommon and
> informally banned in Chromium code.  I am happy to formalize that ban if
> necessary.
> 
> at() explicitly bounds-checks all accesses and throws an exception when the
> check fails.  Doing an explicit check is rarely what people intend or want. 
In
> rare cases it can have performance consequences.  And we generally frown on
> exception-throwing code given that we explicitly compile out exception
support.
> 
> Added to this:
>   * at() is less readable -- it's both more verbose and less idiomatic.
>   * Many people don't truly understand the difference between [] and at()
> (witness scattered random usage of at() we've been cleaning up for years).
> 
> If you want a bounds check, check the bounds yourself; an explicit CHECK or
> DCHECK is far more obvious and is preferable in the rare case that we truly
want
> to always bounds-check.  Or add other appropriate assertions to guarantee code
> behaves as you expect.

[Peter Kasting, 2012-05-31 04:49:25]

LGTM

https://chromiumcodereview.appspot.com/10458045/diff/3/ui/base/models/simple_...
File ui/base/models/simple_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10458045/diff/3/ui/base/models/simple_...
ui/base/models/simple_menu_model.cc:354: void
SimpleMenuModel::ValidateItemIndex(int index) const {
Nit: If you make this method return its argument rather than void, then most of
the callers can inline it to reduce duplicated boilerplate:

int item_index = FlipIndex(index);
ValidateItemIndex(item_index);
do_something(item_index);

-->

do_something(ValidateIndex(FlipIndex(index)));

I find this a little easier to read too.

https://chromiumcodereview.appspot.com/10458045/diff/3/ui/base/models/simple_...
ui/base/models/simple_menu_model.cc:355: CHECK_LE(0, index);
Nit: For inequalities other than NE, go ahead and write in natural reading
order:

CHECK_GE(index, 0);
CHECK_LT(static_cast<size_t>(index), items_.size());

[Lei Zhang, 2012-05-31 06:22:05]

https://chromiumcodereview.appspot.com/10458045/diff/3/ui/base/models/simple_...
File ui/base/models/simple_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10458045/diff/3/ui/base/models/simple_...
ui/base/models/simple_menu_model.cc:354: void
SimpleMenuModel::ValidateItemIndex(int index) const {
On 2012/05/31 04:49:25, Peter Kasting wrote:
> Nit: If you make this method return its argument rather than void, then most
of
> the callers can inline it to reduce duplicated boilerplate:
> 
> int item_index = FlipIndex(index);
> ValidateItemIndex(item_index);
> do_something(item_index);
> 
> -->
> 
> do_something(ValidateIndex(FlipIndex(index)));
> 
> I find this a little easier to read too.

Done.

https://chromiumcodereview.appspot.com/10458045/diff/3/ui/base/models/simple_...
ui/base/models/simple_menu_model.cc:355: CHECK_LE(0, index);
On 2012/05/31 04:49:25, Peter Kasting wrote:
> Nit: For inequalities other than NE, go ahead and write in natural reading
> order:
> 
> CHECK_GE(index, 0);
> CHECK_LT(static_cast<size_t>(index), items_.size());

Done. I always forget you only have to do that with gtest ASSERT_FOO to make the
error messages come out correctly.

[sky, 2012-05-31 14:57:19]

LGTM

[tfarina, 2012-05-31 15:32:25]

Please, also update the CL description and fill the BUG= line with the
correspondent crbug.com/ #.

[Lei Zhang, 2012-05-31 17:07:03]

On 2012/05/31 15:32:25, tfarina wrote:
> Please, also update the CL description and fill the BUG= line with the
> correspondent crbug.com/ #.

The bug description is up to date, but not the thread title. This is not
intended to fix any bugs, but rather just do some cleanup.

[commit-bot: I haz the power, 2012-05-31 17:08:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10458045/3003

[tfarina, 2012-05-31 17:13:36]

On Thu, May 31, 2012 at 2:07 PM,  <thestig@chromium.org> wrote:
> On 2012/05/31 15:32:25, tfarina wrote:
>>
>> Please, also update the CL description and fill the BUG= line with the
>> correspondent crbug.com/ #.
>
>
> The bug description is up to date,
No, it's not. It says: "Cleanup: Use vector::at() in more places in
SimpleMenuModel to catch out of bound accesses." when the patch
doesn't do this.



-- 
Thiago

[Lei Zhang, 2012-05-31 17:16:45]

On 2012/05/31 17:13:36, tfarina wrote:
> On Thu, May 31, 2012 at 2:07 PM,  <mailto:thestig@chromium.org> wrote:
> > On 2012/05/31 15:32:25, tfarina wrote:
> >>
> >> Please, also update the CL description and fill the BUG= line with the
> >> correspondent crbug.com/ #.
> >
> >
> > The bug description is up to date,
> No, it's not. It says: "Cleanup: Use vector::at() in more places in
> SimpleMenuModel to catch out of bound accesses." when the patch
> doesn't do this.

That's the email thread title, which I did not change as to not confuse email
clients. The actual description that will go in the commit message is "Cleanup:
Remove vector::at() calls in SimpleMenuModel. Do CHECKs instead.", as seen in
the web interface.

[Peter Kasting, 2012-05-31 17:33:56]

I went ahead and updated the change title here to make sky happy :)

[tfarina, 2012-05-31 17:35:30]

On Thu, May 31, 2012 at 2:33 PM,  <pkasting@chromium.org> wrote:
> I went ahead and updated the change title here to make sky happy :)
>
Which makes me happy too ;)

> https://chromiumcodereview.appspot.com/10458045/



-- 
Thiago

[commit-bot: I haz the power, 2012-05-31 20:02:03]

Try job failure for 10458045-3003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-01 21:12:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10458045/3003

[commit-bot: I haz the power, 2012-06-02 00:35:31]

Change committed as 140139