Fix UBSan errors (left shift of negative value, left shift overflows int)

webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c

Index: webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c
diff --git a/webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c b/webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c
index c1204ad03adcbcae5175824818ee92f2c24e6928..849834e552e2a0eb007c187f508bcd78efec552a 100644
--- a/webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c
+++ b/webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c
@@ -162,9 +162,9 @@ static void FindInvArSpec(const int16_t* ARCoefQ12,
   }
 
   for (k = 0; k < FRAMESAMPLES / 8; k++) {
-    CurveQ16[FRAMESAMPLES_QUARTER - 1 - k] = CurveQ16[k] -
-        (diffQ16[k] << shftVal);
-    CurveQ16[k] += diffQ16[k] << shftVal;
+    int32_t d = (int32_t)((uint32_t)(diffQ16[k]) << shftVal);

[tlegrand-webrtc, 2016/05/17 12:43:37]

"d" is not a very nice variable name. Can you rename to something more
descriptive, maybe "diffQ16" will do?

[kwiberg-webrtc, 2016/05/17 13:10:39]

Yes, "d" is short and therefore not very descriptive, but its scope is just the
next two lines. It doesn't have to be descriptive, because the definition is
*right there*.

I'll change it if you still don't agree (to "diff_q16_shifted"? "diffQ16" is
already taken), but IMO this will decrease rather than increase legibility.

[tlegrand-webrtc, 2016/05/17 14:01:06]

Yes, sorry, it is of course taken. I've had trouble in the past with short
names, because it is impossible to search for other places where the variable
might be used. And I don't think the style guide has exceptions for variables
only being used on adjacent lines. You normally don't know what will happen with
the code in the future, so that could potentially change. Maybe not in this old
code, but I like consistency.

[kwiberg-webrtc, 2016/05/18 08:21:06]

Yes, but that's not all that relevant for variables with very limited scope
(which is the only case where a short name should be used---otherwise, some uses
of the name will inevitably be far from the definition).
You're right---it doesn't. I'll ask if that's on purpose.
I just feel like this is the same sort of thinking that often gets me in
trouble, writing code that's more general than what's currently needed. If the
additional work (including testing) isn't negligible, and it won't cost extra to
change it if and when the extra generality is needed, it's usually a good idea
to keep your fingers in check and write just what you need at the moment.
Variable names are of course much less work than whole chunks of code, but the
cost of inventing informative variable names is not zero, and they take more
space on the screen and in the mind when you read the code.

Well, enough arguing. I'm landing this CL with a longer variable name now.

+    CurveQ16[FRAMESAMPLES_QUARTER - 1 - k] = CurveQ16[k] - d;
+    CurveQ16[k] += d;

[kwiberg-webrtc, 2016/05/16 14:05:37]

diffQ16[k] was sometimes negative, and left shift of a negative value leads to
undefined behavior. Solve by casting to unsigned, and then back again (both
casts are compile-time-only affairs that are no-ops at runtime).

Also extract the common subexpression, so we don't have to have two copies of
that mouthful.

   }
 }