Improved readability and DCHECKing in AudioVector::[]

Improved readability and DCHECKing in AudioVector::[]

This is a follow-up to https://codereview.webrtc.org/2700633003, where
post-commit comments suggested these changes.

BUG=webrtc:7159
Review-Url: https://codereview.webrtc.org/2706263002
Cr-Commit-Position: refs/heads/master@{#16771}
Committed: https://chromium.googlesource.com/external/webrtc/+/5650a7d1c41a264a0da15c3bb17d2c14335fcc27

[hlundin-webrtc, 2017-02-21 17:01:56]

henrik.lundin@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, nisse@webrtc.org

[hlundin-webrtc, 2017-02-21 17:01:56]

PTAL

FYI: This change seems to produce exactly the same compiler output as the old
code on clang, slightly shorter on MSVC-64 and what looks like quite a lot
shorter on MSVC-32. (Disclaimer: I'm rubbish at reading compiler output.)
https://godbolt.org/g/cJdgfN

[kwiberg-webrtc, 2017-02-21 21:17:13]

lgtm

[nisse-webrtc, 2017-02-22 07:48:40]

lgtm. One note on the overflow check, but I have no concrete suggestion for
changes.

https://codereview.webrtc.org/2706263002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/audio_vector.h (right):

https://codereview.webrtc.org/2706263002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/audio_vector.h:133: RTC_DCHECK_GE(ix, index); 
// Check for overflow.
Given above DCHECKS, this could fail only if capacity > (maximum size_t) / 2.
Which is unlikely to happen (we'd run out of memory), but could be handled
correctly by changing the below condition to 

  if (ix < index || ix >= capacity)
    ix -= capacity;

[kwiberg-webrtc, 2017-02-22 08:25:49]

https://codereview.webrtc.org/2706263002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/audio_vector.h (right):

https://codereview.webrtc.org/2706263002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/audio_vector.h:133: RTC_DCHECK_GE(ix, index); 
// Check for overflow.
On 2017/02/22 07:48:40, nisse-webrtc wrote:
> Given above DCHECKS, this could fail only if capacity > (maximum size_t) / 2.
> Which is unlikely to happen (we'd run out of memory), but could be handled
> correctly by changing the below condition to 
> 
>   if (ix < index || ix >= capacity)
>     ix -= capacity;

Aha. The two DCHECKs on line 130 and 131 guarantee that even if we do have
overflow, subtracting capacity will make everything work.

Doing what you suggest will however (1) cost extra at run-time (the ix < index
check) and (2) make the code harder to read, so given that it's very unlikely to
matter and that this piece of code is performance critical enough that we're
counting instructions for various alternatives, not acting on this clever idea
is probably the right thing to do.

[hlundin-webrtc, 2017-02-22 09:53:23]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2017-02-22 09:53:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hlundin-webrtc, 2017-02-22 09:56:01]

On 2017/02/22 08:25:49, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2706263002/diff/1/webrtc/modules/audio_coding/n...
> File webrtc/modules/audio_coding/neteq/audio_vector.h (right):
> 
>
https://codereview.webrtc.org/2706263002/diff/1/webrtc/modules/audio_coding/n...
> webrtc/modules/audio_coding/neteq/audio_vector.h:133: RTC_DCHECK_GE(ix,
index); 
> // Check for overflow.
> On 2017/02/22 07:48:40, nisse-webrtc wrote:
> > Given above DCHECKS, this could fail only if capacity > (maximum size_t) /
2.
> > Which is unlikely to happen (we'd run out of memory), but could be handled
> > correctly by changing the below condition to 
> > 
> >   if (ix < index || ix >= capacity)
> >     ix -= capacity;
> 
> Aha. The two DCHECKs on line 130 and 131 guarantee that even if we do have
> overflow, subtracting capacity will make everything work.
> 
> Doing what you suggest will however (1) cost extra at run-time (the ix < index
> check) and (2) make the code harder to read, so given that it's very unlikely
to
> matter and that this piece of code is performance critical enough that we're
> counting instructions for various alternatives, not acting on this clever idea
> is probably the right thing to do.

Thanks for the analysis. I'm committing this in its current shape.

[commit-bot: I haz the power, 2017-02-22 10:25:37]

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

[commit-bot: I haz the power, 2017-02-22 10:25:38]

Try jobs failed on following builders:
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21832)

[hlundin-webrtc, 2017-02-22 11:42:19]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2017-02-22 11:42:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 11:45:40]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487763739441080, "parent_rev":
"a14d16ea7dc5d641599bd14d3cd0caceeef807b2", "commit_rev":
"5650a7d1c41a264a0da15c3bb17d2c14335fcc27"}

[commit-bot: I haz the power, 2017-02-22 11:45:43]

Description was changed from

==========
Improved readability and DCHECKing in AudioVector::[]

This is a follow-up to https://codereview.webrtc.org/2700633003, where
post-commit comments suggested these changes.

BUG=webrtc:7159
==========

to

==========
Improved readability and DCHECKing in AudioVector::[]

This is a follow-up to https://codereview.webrtc.org/2700633003, where
post-commit comments suggested these changes.

BUG=webrtc:7159

Review-Url: https://codereview.webrtc.org/2706263002
Cr-Commit-Position: refs/heads/master@{#16771}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5650a7d1c41a264a0da15c3bb...
==========

[commit-bot: I haz the power, 2017-02-22 11:45:43]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/5650a7d1c41a264a0da15c3bb...