Replace some asserts with DCHECKs

Replace some asserts with DCHECKs

NOPRESUBMIT=true
BUG=webrtc:6779
Committed: https://crrev.com/b890c95c33d2fceb8670625ae3e1783bb9bc9a44
Cr-Commit-Position: refs/heads/master@{#15295}

[kwiberg-webrtc, 2016-11-27 00:01:55]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 00:01:57]

Dry run: 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, 2016-11-27 00:05:27]

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

[commit-bot: I haz the power, 2016-11-27 00:05:28]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10675)

[kwiberg-webrtc, 2016-11-27 00:17:12]

Description was changed from

==========
Replace some asserts with DCHECKs

BUG=webrtc:6779
==========

to

==========
Replace some asserts with DCHECKs

NOPRESUBMIT=true
BUG=webrtc:6779
==========

[kwiberg-webrtc, 2016-11-27 00:17:34]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 00:17:37]

Dry run: 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, 2016-11-27 00:20:47]

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

[commit-bot: I haz the power, 2016-11-27 00:20:48]

Dry run: Try jobs failed on following builders:
  linux_arm on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/3345)

[kwiberg-webrtc, 2016-11-27 21:59:04]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 21:59:12]

Dry run: 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, 2016-11-27 22:07:38]

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

[commit-bot: I haz the power, 2016-11-27 22:07:39]

Dry run: Try jobs failed on following builders:
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/3557)

[kwiberg-webrtc, 2016-11-28 05:15:14]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-11-28 05:15:28]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2016-11-28 05:17:48]

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[kwiberg-webrtc, 2016-11-28 05:17:48]

common_audio, common_video, common_types.h, and all the files that were
illegally relying on them to do the #include of assert.h.

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
File webrtc/common_audio/resampler/sinc_resampler.h (right):

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
webrtc/common_audio/resampler/sinc_resampler.h:41: enum : size_t { kKernelSize =
32 };
This is needed because RTC_DCHECK_op takes references to its arguments, I
believe. We could define it in the .cc file instead, but for pure integer
constants the enum hack feels better.

[ossu, 2016-11-28 14:54:25]

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
File webrtc/common_audio/resampler/sinc_resampler.h (right):

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
webrtc/common_audio/resampler/sinc_resampler.h:41: enum : size_t { kKernelSize =
32 };
On 2016/11/28 05:17:48, kwiberg-webrtc wrote:
> This is needed because RTC_DCHECK_op takes references to its arguments, I
> believe. We could define it in the .cc file instead, but for pure integer
> constants the enum hack feels better.

I don't get it. Why can't you take a reference to a static const? And how does
making it an enum help? That should have less storage than the static const.
What errors are you seeing?

[kwiberg-webrtc, 2016-11-28 23:01:47]

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
File webrtc/common_audio/resampler/sinc_resampler.h (right):

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
webrtc/common_audio/resampler/sinc_resampler.h:41: enum : size_t { kKernelSize =
32 };
On 2016/11/28 14:54:25, ossu wrote:
> On 2016/11/28 05:17:48, kwiberg-webrtc wrote:
> > This is needed because RTC_DCHECK_op takes references to its arguments, I
> > believe. We could define it in the .cc file instead, but for pure integer
> > constants the enum hack feels better.
> 
> I don't get it. Why can't you take a reference to a static const? And how does
> making it an enum help? That should have less storage than the static const.
> What errors are you seeing?

IIRC it was a linker error. I believe the template type deduction that takes
place for the RTC_DCHECK_op arguments end up using const T& for the arguments,
which the compiler implements by taking their address if they're lvalues, which
static const class members are. But enum values aren't lvalues (right?), so like
for any other rvalue, a const T& binding will involve the compiler making a
temporary on the stack and taking the address of that.

This godbolt experiment seems to agree with what I just said:
https://godbolt.org/g/BcYr84
Interestingly, there's less code for the static constexpr case than for the enum
case precisely because the compiler doesn't have to generate the code to push
the rvalue onto the stack, so maybe we should in general prefer to use static
constexpr members instead of the enum hack even though it forces us to write a
definition in the .cc file? If so, I should probably do that in this CL.

[ossu, 2016-11-29 10:23:48]

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
File webrtc/common_audio/resampler/sinc_resampler.h (right):

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
webrtc/common_audio/resampler/sinc_resampler.h:41: enum : size_t { kKernelSize =
32 };
On 2016/11/28 23:01:47, kwiberg-webrtc wrote:
> On 2016/11/28 14:54:25, ossu wrote:
> > On 2016/11/28 05:17:48, kwiberg-webrtc wrote:
> > > This is needed because RTC_DCHECK_op takes references to its arguments, I
> > > believe. We could define it in the .cc file instead, but for pure integer
> > > constants the enum hack feels better.
> > 
> > I don't get it. Why can't you take a reference to a static const? And how
does
> > making it an enum help? That should have less storage than the static const.
> > What errors are you seeing?
> 
> IIRC it was a linker error. I believe the template type deduction that takes
> place for the RTC_DCHECK_op arguments end up using const T& for the arguments,
> which the compiler implements by taking their address if they're lvalues,
which
> static const class members are. But enum values aren't lvalues (right?), so
like
> for any other rvalue, a const T& binding will involve the compiler making a
> temporary on the stack and taking the address of that.
> 
> This godbolt experiment seems to agree with what I just said:
> https://godbolt.org/g/BcYr84
> Interestingly, there's less code for the static constexpr case than for the
enum
> case precisely because the compiler doesn't have to generate the code to push
> the rvalue onto the stack, so maybe we should in general prefer to use static
> constexpr members instead of the enum hack even though it forces us to write a
> definition in the .cc file? If so, I should probably do that in this CL.

Ah, I see! So the problem is that kKernelSize doesn't have storage, but for most
cases this is fine because the compiler resolves its value statically. Adding
the line:
  const size_t SincResampler::kKernelSize;
in sinc_resampler.cc should resolve that issue.

Not sure if making it constexpr would solve it as well. Probably not:
regardless, the thing needs a place in memory if we're to take a reference to
it.

[kwiberg-webrtc, 2016-11-29 10:31:15]

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
File webrtc/common_audio/resampler/sinc_resampler.h (right):

https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
webrtc/common_audio/resampler/sinc_resampler.h:41: enum : size_t { kKernelSize =
32 };
On 2016/11/29 10:23:48, ossu wrote:
> On 2016/11/28 23:01:47, kwiberg-webrtc wrote:
> > On 2016/11/28 14:54:25, ossu wrote:
> > > On 2016/11/28 05:17:48, kwiberg-webrtc wrote:
> > > > This is needed because RTC_DCHECK_op takes references to its arguments,
I
> > > > believe. We could define it in the .cc file instead, but for pure
integer
> > > > constants the enum hack feels better.
> > > 
> > > I don't get it. Why can't you take a reference to a static const? And how
> does
> > > making it an enum help? That should have less storage than the static
const.
> > > What errors are you seeing?
> > 
> > IIRC it was a linker error. I believe the template type deduction that takes
> > place for the RTC_DCHECK_op arguments end up using const T& for the
arguments,
> > which the compiler implements by taking their address if they're lvalues,
> which
> > static const class members are. But enum values aren't lvalues (right?), so
> like
> > for any other rvalue, a const T& binding will involve the compiler making a
> > temporary on the stack and taking the address of that.
> > 
> > This godbolt experiment seems to agree with what I just said:
> > https://godbolt.org/g/BcYr84
> > Interestingly, there's less code for the static constexpr case than for the
> enum
> > case precisely because the compiler doesn't have to generate the code to
push
> > the rvalue onto the stack, so maybe we should in general prefer to use
static
> > constexpr members instead of the enum hack even though it forces us to write
a
> > definition in the .cc file? If so, I should probably do that in this CL.
> 
> Ah, I see! So the problem is that kKernelSize doesn't have storage, but for
most
> cases this is fine because the compiler resolves its value statically. Adding
> the line:
>   const size_t SincResampler::kKernelSize;
> in sinc_resampler.cc should resolve that issue.
> 
> Not sure if making it constexpr would solve it as well. Probably not:
> regardless, the thing needs a place in memory if we're to take a reference to
> it.

Yes, exactly. (And note that the godbolt example above uses static constexpr,
confirming that it too has this problem.)

I'll change this CL to use that strategy instead.

[kwiberg-webrtc, 2016-11-29 10:47:39]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 10:47:47]

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

[kwiberg-webrtc, 2016-11-29 10:55:45]

On 2016/11/29 10:31:15, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
> File webrtc/common_audio/resampler/sinc_resampler.h (right):
> 
>
https://codereview.webrtc.org/2535643002/diff/40001/webrtc/common_audio/resam...
> webrtc/common_audio/resampler/sinc_resampler.h:41: enum : size_t { kKernelSize
=
> 32 };
> On 2016/11/29 10:23:48, ossu wrote:
> > On 2016/11/28 23:01:47, kwiberg-webrtc wrote:
> > > On 2016/11/28 14:54:25, ossu wrote:
> > > > On 2016/11/28 05:17:48, kwiberg-webrtc wrote:
> > > > > This is needed because RTC_DCHECK_op takes references to its
arguments,
> I
> > > > > believe. We could define it in the .cc file instead, but for pure
> integer
> > > > > constants the enum hack feels better.
> > > > 
> > > > I don't get it. Why can't you take a reference to a static const? And
how
> > does
> > > > making it an enum help? That should have less storage than the static
> const.
> > > > What errors are you seeing?
> > > 
> > > IIRC it was a linker error. I believe the template type deduction that
takes
> > > place for the RTC_DCHECK_op arguments end up using const T& for the
> arguments,
> > > which the compiler implements by taking their address if they're lvalues,
> > which
> > > static const class members are. But enum values aren't lvalues (right?),
so
> > like
> > > for any other rvalue, a const T& binding will involve the compiler making
a
> > > temporary on the stack and taking the address of that.
> > > 
> > > This godbolt experiment seems to agree with what I just said:
> > > https://godbolt.org/g/BcYr84
> > > Interestingly, there's less code for the static constexpr case than for
the
> > enum
> > > case precisely because the compiler doesn't have to generate the code to
> push
> > > the rvalue onto the stack, so maybe we should in general prefer to use
> static
> > > constexpr members instead of the enum hack even though it forces us to
write
> a
> > > definition in the .cc file? If so, I should probably do that in this CL.
> > 
> > Ah, I see! So the problem is that kKernelSize doesn't have storage, but for
> most
> > cases this is fine because the compiler resolves its value statically.
Adding
> > the line:
> >   const size_t SincResampler::kKernelSize;
> > in sinc_resampler.cc should resolve that issue.
> > 
> > Not sure if making it constexpr would solve it as well. Probably not:
> > regardless, the thing needs a place in memory if we're to take a reference
to
> > it.
> 
> Yes, exactly. (And note that the godbolt example above uses static constexpr,
> confirming that it too has this problem.)
> 
> I'll change this CL to use that strategy instead.

Done. PTAL!

[commit-bot: I haz the power, 2016-11-29 11:14:04]

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

[commit-bot: I haz the power, 2016-11-29 11:14:05]

Dry run: This issue passed the CQ dry run.

[ossu, 2016-11-29 13:25:04]

Great, LGTM!

[kwiberg-webrtc, 2016-11-29 13:30:18]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-11-29 13:30:25]

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, 2016-11-29 13:30:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480426218639210,
"parent_rev": "0d8de37ef9eb36c1fad677749f9b2f74f4ed7519", "commit_rev":
"c865cb2e5b2979f68bcc7e509ee0388db8e61242"}

[commit-bot: I haz the power, 2016-11-29 13:30:44]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-11-29 13:30:52]

Description was changed from

==========
Replace some asserts with DCHECKs

NOPRESUBMIT=true
BUG=webrtc:6779
==========

to

==========
Replace some asserts with DCHECKs

NOPRESUBMIT=true
BUG=webrtc:6779

Committed: https://crrev.com/b890c95c33d2fceb8670625ae3e1783bb9bc9a44
Cr-Commit-Position: refs/heads/master@{#15295}
==========

[commit-bot: I haz the power, 2016-11-29 13:30:54]

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