Replace some asserts with DCHECKs

webrtc/common_audio/resampler/sinc_resampler.h

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 20 matching lines @@
   virtual ~SincResamplerCallback() {}
   virtual void Run(size_t frames, float* destination) = 0;
 };
 
 // SincResampler is a high-quality single-channel sample-rate converter.
 class SincResampler {
  public:
   // The kernel size can be adjusted for quality (higher is better) at the
   // expense of performance.  Must be a multiple of 32.
   // TODO(dalecurtis): Test performance to see if we can jack this up to 64+.
-  static const size_t kKernelSize = 32;
+  enum : size_t { kKernelSize = 32 };

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

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]

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]

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]

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]

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.

 
   // Default request size.  Affects how often and for how much SincResampler
   // calls back for input.  Must be greater than kKernelSize.
   static const size_t kDefaultRequestSize = 512;
 
   // The kernel offset count is used for interpolation and is the number of
   // sub-sample kernel shifts.  Can be adjusted for quality (higher is better)
   // at the expense of allocating more memory.
   static const size_t kKernelOffsetCount = 32;
   static const size_t kKernelStorageSize =
@@ skipping 110 matching lines @@
   float* const r2_;
   float* r3_;
   float* r4_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(SincResampler);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_COMMON_AUDIO_RESAMPLER_SINC_RESAMPLER_H_