Further optimization of AudioVector::operator[]

webrtc/modules/audio_coding/neteq/audio_vector.h

Index: webrtc/modules/audio_coding/neteq/audio_vector.h
diff --git a/webrtc/modules/audio_coding/neteq/audio_vector.h b/webrtc/modules/audio_coding/neteq/audio_vector.h
index 18ab95c46d717bdd04a1c0db0557a1d5f90f405a..9100ff896893ebd43e53b7dbb4cfede2f10b48e5 100644
--- a/webrtc/modules/audio_coding/neteq/audio_vector.h
+++ b/webrtc/modules/audio_coding/neteq/audio_vector.h
@@ -128,9 +128,8 @@ class AudioVector {
                                  size_t begin_index,
                                  size_t capacity) {
     RTC_DCHECK_GE(begin_index + index, index);  // Check for overflow.
-    const size_t ix = begin_index + index >= capacity
-                          ? begin_index + index - capacity
-                          : begin_index + index;
+    const size_t ix =
+        begin_index + index - (begin_index + index >= capacity ? capacity : 0);

[nisse-webrtc, 2017/02/17 08:28:25]

I'm guess the compiler can recognize (begin_index + index) as a common
subexpession, but I think the code would be more readable if assigning it to a
variable. Like

size_t ix = begin_index + index;
RTC_DCHECK_GE(ix, index);
if (ix >= capacity) ix -= capacity; 

Or some variation of the last line to get branch free code from relevant
compilers. It ought to be compiled to the sequence move, sub, cmov, unless the
compiler is more clever than I ;-)

[kwiberg-webrtc, 2017/02/17 09:43:23]

Yes, I agree that is more readable. I don't like mutating variables, but in this
case it actually is better.

(All the relevant compilers were able to eliminate the common subexpression in
the old code, however, so this change would be purely for the humans.)

     RTC_DCHECK_LT(ix, capacity);
     return ix;
   }