Match existing type usage better.

Match existing type usage better.

This makes a variety of small changes to synchronize bits of code using different types, remove useless code or casts, and add explicit casts in some places previously doing implicit ones.  For example:

* Change a few type declarations to better match how the majority of code uses those objects.
* Eliminate "< 0" check for unsigned values.
* Replace "(float)sin(x)", where |x| is also a float, with "sinf(x)", and similar.
* Add casts to uint32_t in many places timestamps were used and the existing code stored signed values into the unsigned objects.
* Remove downcasts when the results would be passed to a larger type, e.g. calling "foo((int16_t)x)" with an int |x| when foo() takes an int instead of an int16_t.
* Similarly, add casts when passing a larger type to a function taking a smaller one.
* Add casts to int16_t when doing something like "int16_t = int16_t + int16_t" as the "+" operation would implicitly upconvert to int, and similar.
* Use "false" instead of "0" for setting a bool.
* Shift a few temp types when doing a multi-stage calculation involving typecasts, so as to put the most logical/semantically correct type possible into the temps.  For example, when doing "int foo = int + int; size_t bar = (size_t)foo + size_t;", we might change |foo| to a size_t and move the cast if it makes more sense for |foo| to be represented as a size_t.

BUG=none
R=andrew@webrtc.org, asapersson@webrtc.org, henrika@webrtc.org, juberti@webrtc.org, kwiberg@webrtc.org
TBR=andrew, asapersson, henrika

Committed: https://chromium.googlesource.com/external/webrtc/+/b7e5054414ff524f9db81dab7917729b8c4c8bcb

[Peter Kasting, 2015-06-05 21:40:36]

pkasting@chromium.org changed reviewers:
+ andrew@webrtc.org, asapersson@webrtc.org, henrika@webrtc.org,
juberti@webrtc.org, kwiberg@webrtc.org

[Peter Kasting, 2015-06-05 21:41:36]

This is a reupload of https://webrtc-codereview.appspot.com/55529004/ .

TBRing to henrika, andrew, and asapersson since they already LGTMed the original
change.

Still needs review from:

juberti: talk/app/webrtc/ OWNERS
kwiberg: webrtc/modules/audio_coding/ OWNERS

[Andrew MacDonald, 2015-06-05 23:47:39]

lgtm

[kwiberg-webrtc, 2015-06-08 13:09:38]

Sorry for taking so long.

I'm not really a fan of having explicit casts in lots of places where the
compiler will predictably generate the same cast implicitly. Why is it a good
idea?

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc
(right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc:80:
timestamp_ += static_cast<uint32_t>(num_audio_samples_10ms_);
checked_cast? Otherwise, I don't really see what this explicit cast buys us.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c:373: SIDdata[0] =
(uint8_t)index;
Why is this sort of cast net useful? It has the advantage of communicating that
the author wanted to indicate that the value's type was too large (or similar),
but the compiler doesn't verify it, and with the cast also won't catch a bunch
of other mistakes. You can also no longer tell by looking at the program text
whether a cast is required or just decoration (since both classes are now
nonempty).

This argument is slightly weaker in C++, where casts aren't as powerful.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c:118:
max16=WebRtcSpl_MaxAbsValueW16(&regressor[-50], ENH_BLOCKL_HALF + 50 - 1);
Optional: fix formatting on the whole line while you're touching it.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
checked_cast, so that it will actually fail if out of bounds?

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c
(right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c:73:
BN_data->send_time += current_framesamples;
Tabs.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:101: double
tmpSumStreamLen = 0;
0.0 when you assign to a double, for clarity?

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:391:
tmpSumStreamLen = 0;
0.0 here too? (And in any more places?)

[Peter Kasting, 2015-06-08 20:44:29]

New snap not up, just replying to questions for now.

On 2015/06/08 13:09:38, kwiberg1 wrote:
> Sorry for taking so long.
> 
> I'm not really a fan of having explicit casts in lots of places where the
> compiler will predictably generate the same cast implicitly. Why is it a good
> idea?

The locations I'm adding casts to will, if left without explicit casts, generate
truncation warning on at least MSVC -- except that we've disabled such warnings
because our codebase has too many.  Re-enabling that warning is the reason why
I'm making all these refactoring changes to begin with.

The warning is frequently benign, but has caught places where we didn't intend
for types to not match, and it's reasonable to insert explicit casts in the
intentional places as a way of telling the reader that a cast is, in fact,
occurring, and the author meant for that to be the case.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c:373: SIDdata[0] =
(uint8_t)index;
On 2015/06/08 13:09:37, kwiberg1 wrote:
> Why is this sort of cast net useful? It has the advantage of communicating
that
> the author wanted to indicate that the value's type was too large (or
similar),
> but the compiler doesn't verify it,

By "the compiler doesn't verify it", do you mean that you're concerned about
missing some sort of value-propagation-based warning that might happen today
about casting an out-of-range value?

If so, I don't believe the places I'm modifying really run this risk -- I don't
think the compiler is sophisticated enough to do compile-time analysis to
determine the complete range of possible values here and produce such warnings,
and we're not currently generating run-time range checks in such code either. 
So I don't think there's any loss of warning coverage.

> and with the cast also won't catch a bunch
> of other mistakes.

Can you be more specific what mistakes adding this cast prevents catching?  If
you're concerned about the compiler catching downcasts from larger types --
that's precisely the warning I'm trying to enable, which found this to begin
with, and for which I manually inspected the code and verified that it's safe. 
Without adding these sorts of casts in the safe locations, we can't enable such
a warning, so we can't catch these at all.

> You can also no longer tell by looking at the program text
> whether a cast is required or just decoration (since both classes are now
> nonempty).

Are you saying, you can't tell whether the cast is required in order to compile
cleanly?  The idea is to enable warnings such that this is, in fact, required in
order to compile cleanly.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c:118:
max16=WebRtcSpl_MaxAbsValueW16(&regressor[-50], ENH_BLOCKL_HALF + 50 - 1);
On 2015/06/08 13:09:38, kwiberg1 wrote:
> Optional: fix formatting on the whole line while you're touching it.

I can do that for all three places in this file.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/08 13:09:38, kwiberg1 wrote:
> checked_cast, so that it will actually fail if out of bounds?

Can this really ever be out-of-bounds in this test?  That seems extraordinarily
unlikely given that the sizes involved should all be very small.

I'm a fan of checked_casts for cases where overflow is in theory possible and
would lead to security problems and other catastrophic issues, but in this case
I don't think it can happen, and if it did, we would simply have some test code
that would get the wrong arrival time in the model, which seems reasonably
benign.

So I'm concerned that using checked_cast misleads the reader into thinking
failure is both more likely and more important here than it really is.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c
(right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c:73:
BN_data->send_time += current_framesamples;
On 2015/06/08 13:09:38, kwiberg1 wrote:
> Tabs.

Oops, this must have been a merge failure.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:101: double
tmpSumStreamLen = 0;
On 2015/06/08 13:09:38, kwiberg1 wrote:
> 0.0 when you assign to a double, for clarity?

I can if you really want, though I'm not a big fan.

As you sort of allude to below, "= 0" is fairly common throughout our codebase
-- "= 0.0" is exceptional.  And "= 0.0" implies to me that it was important not
to say "= 0", which is what I'd have normally expected the code to read; like,
maybe somehow "= 0" would result in a run-time value conversion of int to
double, or "0" alone wasn't precise enough.  Since neither is true, it calls my
eye to the code for a difference that doesn't actually matter.

ALl that said, my opposition to this is weak, so if you still think 0.0 is
better, I'll do that.

[kwiberg-webrtc, 2015-06-08 23:58:25]

On 2015/06/08 20:44:29, Peter Kasting wrote:
> New snap not up, just replying to questions for now.
> 
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > Sorry for taking so long.
> > 
> > I'm not really a fan of having explicit casts in lots of places where the
> > compiler will predictably generate the same cast implicitly. Why is it a
good
> > idea?
> 
> The locations I'm adding casts to will, if left without explicit casts,
generate
> truncation warning on at least MSVC -- except that we've disabled such
warnings
> because our codebase has too many.  Re-enabling that warning is the reason why
> I'm making all these refactoring changes to begin with.
> 
> The warning is frequently benign, but has caught places where we didn't intend
> for types to not match, and it's reasonable to insert explicit casts in the
> intentional places as a way of telling the reader that a cast is, in fact,
> occurring, and the author meant for that to be the case.

Mmm. I generally disagree that MSVC is giving us a good cost/benefit ratio with
this warning even though it's of course not all false positives, but I can live
with enabling it. At least that means a machine is checking that there are casts
in all the right places.

> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c (right):
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c:373: SIDdata[0] =
> (uint8_t)index;
> On 2015/06/08 13:09:37, kwiberg1 wrote:
> > Why is this sort of cast net useful? It has the advantage of communicating
> that
> > the author wanted to indicate that the value's type was too large (or
> similar),
> > but the compiler doesn't verify it,
> 
> By "the compiler doesn't verify it", do you mean that you're concerned about
> missing some sort of value-propagation-based warning that might happen today
> about casting an out-of-range value?
> 
> If so, I don't believe the places I'm modifying really run this risk -- I
don't
> think the compiler is sophisticated enough to do compile-time analysis to
> determine the complete range of possible values here and produce such
warnings,
> and we're not currently generating run-time range checks in such code either. 
> So I don't think there's any loss of warning coverage.
> 
> > and with the cast also won't catch a bunch
> > of other mistakes.
> 
> Can you be more specific what mistakes adding this cast prevents catching?  If
> you're concerned about the compiler catching downcasts from larger types --
> that's precisely the warning I'm trying to enable, which found this to begin
> with, and for which I manually inspected the code and verified that it's safe.

> Without adding these sorts of casts in the safe locations, we can't enable
such
> a warning, so we can't catch these at all.
> 
> > You can also no longer tell by looking at the program text
> > whether a cast is required or just decoration (since both classes are now
> > nonempty).
> 
> Are you saying, you can't tell whether the cast is required in order to
compile
> cleanly?  The idea is to enable warnings such that this is, in fact, required
in
> order to compile cleanly.
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c (right):
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c:118:
> max16=WebRtcSpl_MaxAbsValueW16(&regressor[-50], ENH_BLOCKL_HALF + 50 - 1);
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > Optional: fix formatting on the whole line while you're touching it.
> 
> I can do that for all three places in this file.
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
> BN_data->arrival_time += static_cast<uint32_t>(
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > checked_cast, so that it will actually fail if out of bounds?
> 
> Can this really ever be out-of-bounds in this test?  That seems
extraordinarily
> unlikely given that the sizes involved should all be very small.
> 
> I'm a fan of checked_casts for cases where overflow is in theory possible and
> would lead to security problems and other catastrophic issues, but in this
case
> I don't think it can happen, and if it did, we would simply have some test
code
> that would get the wrong arrival time in the model, which seems reasonably
> benign.
> 
> So I'm concerned that using checked_cast misleads the reader into thinking
> failure is both more likely and more important here than it really is.
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c
> (right):
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c:73:
> BN_data->send_time += current_framesamples;
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > Tabs.
> 
> Oops, this must have been a merge failure.
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):
> 
>
https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:101: double
> tmpSumStreamLen = 0;
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > 0.0 when you assign to a double, for clarity?
> 
> I can if you really want, though I'm not a big fan.
> 
> As you sort of allude to below, "= 0" is fairly common throughout our codebase
> -- "= 0.0" is exceptional.  And "= 0.0" implies to me that it was important
not
> to say "= 0", which is what I'd have normally expected the code to read; like,
> maybe somehow "= 0" would result in a run-time value conversion of int to
> double, or "0" alone wasn't precise enough.  Since neither is true, it calls
my
> eye to the code for a difference that doesn't actually matter.
> 
> ALl that said, my opposition to this is weak, so if you still think 0.0 is
> better, I'll do that.

[kwiberg-webrtc, 2015-06-09 00:48:12]

lgtm for webrtc/modules/audio_coding/

because in the end it's more productive to yield to MSVC than to fight it. Still
not happy, though. :-/

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c:373: SIDdata[0] =
(uint8_t)index;
On 2015/06/08 20:44:28, Peter Kasting wrote:
> On 2015/06/08 13:09:37, kwiberg1 wrote:
> > Why is this sort of cast net useful? It has the advantage of
> > communicating that the author wanted to indicate that the value's
> > type was too large (or similar), but the compiler doesn't verify
> > it,
> 
> By "the compiler doesn't verify it", do you mean that you're
> concerned about missing some sort of value-propagation-based warning
> that might happen today about casting an out-of-range value?

No, I meant that the compiler only verifies that the cast is legal,
not that it is needed, or that it is equivalent to the implicit cast
the compiler would have used had the explicit cast not been there.

> > and with the cast also won't catch a bunch of other mistakes.
> 
> Can you be more specific what mistakes adding this cast prevents
> catching?  If you're concerned about the compiler catching downcasts
> from larger types -- that's precisely the warning I'm trying to
> enable, which found this to begin with, and for which I manually
> inspected the code and verified that it's safe.  Without adding
> these sorts of casts in the safe locations, we can't enable such a
> warning, so we can't catch these at all.

The risks I'm worried about are largely the ones I mention above: that
the compiler doesn't verify that the cast is needed, or that it is
equivalent to the implicit cast the compiler would have used had the
explicit cast not been there. This means, for example, that you can
cast to a too-small type without the compiler complaining. I'm sure it
doesn't happen now, since you added the casts with the help of the
compiler, but a later refactoring could silently break things.

> > You can also no longer tell by looking at the program text whether
> > a cast is required or just decoration (since both classes are now
> > nonempty).
> 
> Are you saying, you can't tell whether the cast is required in order
> to compile cleanly?  The idea is to enable warnings such that this
> is, in fact, required in order to compile cleanly.

This objection is rendered somewhat moot since you do in fact have a
machine telling you where to insert casts (although I'd personally
classify these casts as "humoring the compiler" rather than
"required").

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/08 20:44:29, Peter Kasting wrote:
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > checked_cast, so that it will actually fail if out of bounds?
> 
> Can this really ever be out-of-bounds in this test?  That seems
extraordinarily
> unlikely given that the sizes involved should all be very small.
> 
> I'm a fan of checked_casts for cases where overflow is in theory possible and
> would lead to security problems and other catastrophic issues, but in this
case
> I don't think it can happen, and if it did, we would simply have some test
code
> that would get the wrong arrival time in the model, which seems reasonably
> benign.
> 
> So I'm concerned that using checked_cast misleads the reader into thinking
> failure is both more likely and more important here than it really is.

I see your point. dchecked_cast, then? (Yes, I know it doesn't exist yet.)

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:101: double
tmpSumStreamLen = 0;
On 2015/06/08 20:44:29, Peter Kasting wrote:
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > 0.0 when you assign to a double, for clarity?
> 
> I can if you really want, though I'm not a big fan.
> 
> As you sort of allude to below, "= 0" is fairly common throughout our codebase
> -- "= 0.0" is exceptional.  And "= 0.0" implies to me that it was important
not
> to say "= 0", which is what I'd have normally expected the code to read; like,
> maybe somehow "= 0" would result in a run-time value conversion of int to
> double, or "0" alone wasn't precise enough.  Since neither is true, it calls
my
> eye to the code for a difference that doesn't actually matter.
> 
> ALl that said, my opposition to this is weak, so if you still think 0.0 is
> better, I'll do that.

The WebRTC audio code is littered with float literals such as 0.f, but if that's
not the case in general then I'm happy to use just 0 here (since, as you say,
the compiler will not care either way---the only consideration is human
readability).

[Peter Kasting, 2015-06-09 01:18:13]

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

[Peter Kasting, 2015-06-09 01:18:14]

Hey Nico, I've added you as my go-to clang guy.  Do you think the warnings I
mention below would be useful?  If yes, how would I go about getting them added?

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c:373: SIDdata[0] =
(uint8_t)index;
On 2015/06/09 00:48:12, kwiberg1 wrote:
> On 2015/06/08 20:44:28, Peter Kasting wrote:
> > On 2015/06/08 13:09:37, kwiberg1 wrote:
> > > Why is this sort of cast net useful? It has the advantage of
> > > communicating that the author wanted to indicate that the value's
> > > type was too large (or similar), but the compiler doesn't verify
> > > it,
> > 
> > By "the compiler doesn't verify it", do you mean that you're
> > concerned about missing some sort of value-propagation-based warning
> > that might happen today about casting an out-of-range value?
> 
> No, I meant that the compiler only verifies that the cast is legal,
> not that it is needed, or that it is equivalent to the implicit cast
> the compiler would have used had the explicit cast not been there.

Right, so this would be two issues:

(1) Adding an explicit cast to type T when the source is already of type T
(2) Casting to type T when the destination is actually of type U

For (2), at least, if T is larger than U, the compiler can still catch this.  If
it's smaller, though, it won't.

I wonder if we could get clang warnings about both these things.  (1) should
just be deleted, while (2) suggests the code may have a bug, possibly due to
refactoring.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/09 00:48:12, kwiberg1 wrote:
> On 2015/06/08 20:44:29, Peter Kasting wrote:
> > On 2015/06/08 13:09:38, kwiberg1 wrote:
> > > checked_cast, so that it will actually fail if out of bounds?
> > 
> > Can this really ever be out-of-bounds in this test?  That seems
> extraordinarily
> > unlikely given that the sizes involved should all be very small.
> > 
> > I'm a fan of checked_casts for cases where overflow is in theory possible
and
> > would lead to security problems and other catastrophic issues, but in this
> case
> > I don't think it can happen, and if it did, we would simply have some test
> code
> > that would get the wrong arrival time in the model, which seems reasonably
> > benign.
> > 
> > So I'm concerned that using checked_cast misleads the reader into thinking
> > failure is both more likely and more important here than it really is.
> 
> I see your point. dchecked_cast, then? (Yes, I know it doesn't exist yet.)

We could do this if you'd like:

  int arrival_time = ((packet_size + HeaderSize) * 8 * FS) / (bottleneck +
HeaderRate));
  DCHECK(rtc::IsValueInRangeForNumericType<uint32_t>(arrival_time))
  BN_data->arrival_time += static_cast<uint32_t>(arrival_time);

[kwiberg-webrtc, 2015-06-09 01:28:26]

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/webrtc_cng.c:373: SIDdata[0] =
(uint8_t)index;
On 2015/06/09 01:18:14, Peter Kasting wrote:
> On 2015/06/09 00:48:12, kwiberg1 wrote:
> > On 2015/06/08 20:44:28, Peter Kasting wrote:
> > > On 2015/06/08 13:09:37, kwiberg1 wrote:
> > > > Why is this sort of cast net useful? It has the advantage of
> > > > communicating that the author wanted to indicate that the value's
> > > > type was too large (or similar), but the compiler doesn't verify
> > > > it,
> > > 
> > > By "the compiler doesn't verify it", do you mean that you're
> > > concerned about missing some sort of value-propagation-based warning
> > > that might happen today about casting an out-of-range value?
> > 
> > No, I meant that the compiler only verifies that the cast is legal,
> > not that it is needed, or that it is equivalent to the implicit cast
> > the compiler would have used had the explicit cast not been there.
> 
> Right, so this would be two issues:
> 
> (1) Adding an explicit cast to type T when the source is already of type T
> (2) Casting to type T when the destination is actually of type U
> 
> For (2), at least, if T is larger than U, the compiler can still catch this. 
If
> it's smaller, though, it won't.
> 
> I wonder if we could get clang warnings about both these things.  (1) should
> just be deleted, while (2) suggests the code may have a bug, possibly due to
> refactoring.

Those sound very useful to me. Not sure how much of a problem there would be
with false positives, though.

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/09 01:18:14, Peter Kasting wrote:
> On 2015/06/09 00:48:12, kwiberg1 wrote:
> > On 2015/06/08 20:44:29, Peter Kasting wrote:
> > > On 2015/06/08 13:09:38, kwiberg1 wrote:
> > > > checked_cast, so that it will actually fail if out of bounds?
> > > 
> > > Can this really ever be out-of-bounds in this test?  That seems
> > extraordinarily
> > > unlikely given that the sizes involved should all be very small.
> > > 
> > > I'm a fan of checked_casts for cases where overflow is in theory possible
> and
> > > would lead to security problems and other catastrophic issues, but in this
> > case
> > > I don't think it can happen, and if it did, we would simply have some test
> > code
> > > that would get the wrong arrival time in the model, which seems reasonably
> > > benign.
> > > 
> > > So I'm concerned that using checked_cast misleads the reader into thinking
> > > failure is both more likely and more important here than it really is.
> > 
> > I see your point. dchecked_cast, then? (Yes, I know it doesn't exist yet.)
> 
> We could do this if you'd like:
> 
>   int arrival_time = ((packet_size + HeaderSize) * 8 * FS) / (bottleneck +
> HeaderRate));
>   DCHECK(rtc::IsValueInRangeForNumericType<uint32_t>(arrival_time))
>   BN_data->arrival_time += static_cast<uint32_t>(arrival_time);

Too much work, and too verbose. It'll only really work well if packaged up as
dchecked_cast or similar.

[Peter Kasting, 2015-06-09 02:10:13]

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/09 01:28:26, kwiberg1 wrote:
> On 2015/06/09 01:18:14, Peter Kasting wrote:
> > On 2015/06/09 00:48:12, kwiberg1 wrote:
> > > On 2015/06/08 20:44:29, Peter Kasting wrote:
> > > > On 2015/06/08 13:09:38, kwiberg1 wrote:
> > > > > checked_cast, so that it will actually fail if out of bounds?
> > > > 
> > > > Can this really ever be out-of-bounds in this test?  That seems
> > > extraordinarily
> > > > unlikely given that the sizes involved should all be very small.
> > > > 
> > > > I'm a fan of checked_casts for cases where overflow is in theory
possible
> > and
> > > > would lead to security problems and other catastrophic issues, but in
this
> > > case
> > > > I don't think it can happen, and if it did, we would simply have some
test
> > > code
> > > > that would get the wrong arrival time in the model, which seems
reasonably
> > > > benign.
> > > > 
> > > > So I'm concerned that using checked_cast misleads the reader into
thinking
> > > > failure is both more likely and more important here than it really is.
> > > 
> > > I see your point. dchecked_cast, then? (Yes, I know it doesn't exist yet.)
> > 
> > We could do this if you'd like:
> > 
> >   int arrival_time = ((packet_size + HeaderSize) * 8 * FS) / (bottleneck +
> > HeaderRate));
> >   DCHECK(rtc::IsValueInRangeForNumericType<uint32_t>(arrival_time))
> >   BN_data->arrival_time += static_cast<uint32_t>(arrival_time);
> 
> Too much work, and too verbose. It'll only really work well if packaged up as
> dchecked_cast or similar.

Hmmm.  I'm not terribly keen on adding something called "dchecked_cast" as it
sounds as if it happens in the same scenarios DCHECK is on, when in fact the
cast happens all the time.

If you're really super-keen on this I could ask the security folks that put in
checked_cast what they think.  Personally I'm inclined to leave this one as the
static_cast.

[kwiberg-webrtc, 2015-06-09 10:56:31]

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.webrtc.org/1168753002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/09 02:10:13, Peter Kasting wrote:
> Hmmm. I'm not terribly keen on adding something called
> "dchecked_cast" as it sounds as if it happens in the same scenarios
> DCHECK is on, when in fact the cast happens all the time.

I would have thought it was obvious---there's only one reasonable
thing it could do, and casting or not depending on DCHECK_IS_ON isn't
one of them. But then I thought of the name, so I would say that...

> If you're really super-keen on this I could ask the security folks
> that put in checked_cast what they think. Personally I'm inclined to
> leave this one as the static_cast.

I guess you can leave it. But this isn't the first time I've wanted
dchecked_cast, so chances are I'll get around to building it one of
these days.

[henrika_webrtc, 2015-06-09 14:55:45]

LGTM

[Peter Kasting, 2015-06-10 01:09:53]

pkasting@chromium.org changed reviewers:
- thakis@chromium.org

[Peter Kasting, 2015-06-10 01:09:54]

New snap up.  Justin, I'm just waiting on your LGTM now.

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c (right):

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/ilbc/enhancer_interface.c:118:
max16=WebRtcSpl_MaxAbsValueW16(&regressor[-50], ENH_BLOCKL_HALF + 50 - 1);
On 2015/06/08 20:44:29, Peter Kasting wrote:
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > Optional: fix formatting on the whole line while you're touching it.
> 
> I can do that for all three places in this file.

Done.

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc (right):

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/test/kenny.cc:65:
BN_data->arrival_time += static_cast<uint32_t>(
On 2015/06/09 10:56:31, kwiberg1 wrote:
> On 2015/06/09 02:10:13, Peter Kasting wrote:
> > Hmmm. I'm not terribly keen on adding something called
> > "dchecked_cast" as it sounds as if it happens in the same scenarios
> > DCHECK is on, when in fact the cast happens all the time.
> 
> I would have thought it was obvious---there's only one reasonable
> thing it could do, and casting or not depending on DCHECK_IS_ON isn't
> one of them. But then I thought of the name, so I would say that...
> 
> > If you're really super-keen on this I could ask the security folks
> > that put in checked_cast what they think. Personally I'm inclined to
> > leave this one as the static_cast.
> 
> I guess you can leave it. But this isn't the first time I've wanted
> dchecked_cast, so chances are I'll get around to building it one of
> these days.

Acknowledged.

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c
(right):

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/test/test_iSACfixfloat.c:73:
BN_data->send_time += current_framesamples;
On 2015/06/08 20:44:29, Peter Kasting wrote:
> On 2015/06/08 13:09:38, kwiberg1 wrote:
> > Tabs.
> 
> Oops, this must have been a merge failure.

Done.

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.chromium.org/1168753002/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:101: double
tmpSumStreamLen = 0;
On 2015/06/09 00:48:12, kwiberg1 wrote:
> On 2015/06/08 20:44:29, Peter Kasting wrote:
> > On 2015/06/08 13:09:38, kwiberg1 wrote:
> > > 0.0 when you assign to a double, for clarity?
> > 
> > I can if you really want, though I'm not a big fan.
> > 
> > As you sort of allude to below, "= 0" is fairly common throughout our
codebase
> > -- "= 0.0" is exceptional.  And "= 0.0" implies to me that it was important
> not
> > to say "= 0", which is what I'd have normally expected the code to read;
like,
> > maybe somehow "= 0" would result in a run-time value conversion of int to
> > double, or "0" alone wasn't precise enough.  Since neither is true, it calls
> my
> > eye to the code for a difference that doesn't actually matter.
> > 
> > ALl that said, my opposition to this is weak, so if you still think 0.0 is
> > better, I'll do that.
> 
> The WebRTC audio code is littered with float literals such as 0.f, but if
that's
> not the case in general then I'm happy to use just 0 here (since, as you say,
> the compiler will not care either way---the only consideration is human
> readability).

Acknowledged.

[juberti1, 2015-06-10 05:40:54]

https://codereview.webrtc.org/1168753002/diff/40001/talk/app/webrtc/test/fake...
File talk/app/webrtc/test/fakeaudiocapturemodule.cc (right):

https://codereview.webrtc.org/1168753002/diff/40001/talk/app/webrtc/test/fake...
talk/app/webrtc/test/fakeaudiocapturemodule.cc:48: static const uint8_t
kNumberOfChannels = 1;
Generally I think that using sized types for things like this is a bad idea, and
we should use plain old int instead, although I note that the NeedMorePlayData
API expects such a type.

[åsapersson, 2015-06-10 06:46:53]

lgtm

[Peter Kasting, 2015-06-11 04:03:46]

https://codereview.webrtc.org/1168753002/diff/40001/talk/app/webrtc/test/fake...
File talk/app/webrtc/test/fakeaudiocapturemodule.cc (right):

https://codereview.webrtc.org/1168753002/diff/40001/talk/app/webrtc/test/fake...
talk/app/webrtc/test/fakeaudiocapturemodule.cc:48: static const uint8_t
kNumberOfChannels = 1;
On 2015/06/10 05:40:53, juberti1 wrote:
> Generally I think that using sized types for things like this is a bad idea,
and
> we should use plain old int instead, although I note that the NeedMorePlayData
> API expects such a type.

Yes, and that's why I changed this -- all the places it's used it's being passed
to an API expecting a uint8_t.

I could try to change that API, and everyone that calls it and that it calls,
etc. -- I'm not opposed to that, and indeed, doing that sort of thing for other
APIs (with size_t) is what all this work is leading up to -- but that's a bigger
change (I don't know how big without pulling the thread and seeing what
unravels) and I preferred in the meantime to eliminate the compiler downcast
warning and make the type usage consistent today.

So even if I were to make the API change there, I'd still want to do this in the
meantime unless I was making the change now.

Is that OK?

[juberti1, 2015-06-11 05:45:59]

On 2015/06/11 at 04:03:46, pkasting wrote:
>
https://codereview.webrtc.org/1168753002/diff/40001/talk/app/webrtc/test/fake...
> File talk/app/webrtc/test/fakeaudiocapturemodule.cc (right):
> 
>
https://codereview.webrtc.org/1168753002/diff/40001/talk/app/webrtc/test/fake...
> talk/app/webrtc/test/fakeaudiocapturemodule.cc:48: static const uint8_t
kNumberOfChannels = 1;
> On 2015/06/10 05:40:53, juberti1 wrote:
> > Generally I think that using sized types for things like this is a bad idea,
and
> > we should use plain old int instead, although I note that the
NeedMorePlayData
> > API expects such a type.
> 
> Yes, and that's why I changed this -- all the places it's used it's being
passed to an API expecting a uint8_t.
> 
> I could try to change that API, and everyone that calls it and that it calls,
etc. -- I'm not opposed to that, and indeed, doing that sort of thing for other
APIs (with size_t) is what all this work is leading up to -- but that's a bigger
change (I don't know how big without pulling the thread and seeing what
unravels) and I preferred in the meantime to eliminate the compiler downcast
warning and make the type usage consistent today.
> 
> So even if I were to make the API change there, I'd still want to do this in
the meantime unless I was making the change now.
> 
> Is that OK?

lgtm

Yes, that sounds good to me. I'm sure there are many other places where we use
sized types that should really be int or size_t.

[commit-bot: I haz the power, 2015-06-11 19:56:10]

Patchset 4 (id:??) landed as
https://crrev.com/b7e5054414ff524f9db81dab7917729b8c4c8bcb
Cr-Commit-Position: refs/heads/master@{#9419}

[Peter Kasting, 2015-06-11 19:56:13]

Committed patchset #4 (id:60001) manually as
b7e5054414ff524f9db81dab7917729b8c4c8bcb (presubmit successful).