Cleanup some audio classes with C++11 shine!

Cleanup some audio classes with C++11 shine!

Use newly allowed features to cleanup some audio code:
- auto for iterator based for loop.
- nullptr for NULL.

Additionally const& cleanup found while cleaning.

BUG=none
TEST=unittests pass.

Committed: https://crrev.com/1dbc64a018d46617f2802844dc9080f0cc3aacc2
Cr-Commit-Position: refs/heads/master@{#297048}

[DaleCurtis, 2014-09-25 19:58:28]

dalecurtis@chromium.org changed reviewers:
+ wolenetz@chromium.org

[DaleCurtis, 2014-09-25 19:58:28]

Alternatively titled: perfcrastinating.

[DaleCurtis, 2014-09-25 19:59:26]

( http://chromium-cpp.appspot.com/ lists these as approved with the
recommendation to try them out and have fun. wee! \o/ )

[wolenetz, 2014-09-26 01:22:31]

lgtm

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
media/base/audio_renderer_mixer.cc:61: for (ErrorCallbackList::iterator it =
error_callbacks_.begin();
I wish for a range-based equivalent here, but we need the iterator for erase.
Any better ideas that the C++11 stuff could give us here?

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
media/base/audio_renderer_mixer.cc:97: for (const auto& cb : error_callbacks_)
aside (I'm filing a crbug): rietveld's lint doesn't like C++11: "Storage class
(static, extern, typedef, etc) should be first."

https://codereview.chromium.org/602193003/diff/1/media/base/audio_splicer.cc
File media/base/audio_splicer.cc (right):

https://codereview.chromium.org/602193003/diff/1/media/base/audio_splicer.cc#...
media/base/audio_splicer.cc:217: for (const auto& buffer : output_buffers_)
aside ditto: (rietveld lint C++11 bug...)
"Storage class (static, extern, typedef, etc) should be first."

[wolenetz, 2014-09-26 01:31:09]

On 2014/09/26 01:22:31, wolenetz wrote:
> lgtm
> 
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
> File media/base/audio_renderer_mixer.cc (right):
> 
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
> media/base/audio_renderer_mixer.cc:61: for (ErrorCallbackList::iterator it =
> error_callbacks_.begin();
> I wish for a range-based equivalent here, but we need the iterator for erase.
> Any better ideas that the C++11 stuff could give us here?
> 
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
> media/base/audio_renderer_mixer.cc:97: for (const auto& cb : error_callbacks_)
> aside (I'm filing a crbug): rietveld's lint doesn't like C++11: "Storage class
> (static, extern, typedef, etc) should be first."
> 
> https://codereview.chromium.org/602193003/diff/1/media/base/audio_splicer.cc
> File media/base/audio_splicer.cc (right):
> 
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_splicer.cc#...
> media/base/audio_splicer.cc:217: for (const auto& buffer : output_buffers_)
> aside ditto: (rietveld lint C++11 bug...)
> "Storage class (static, extern, typedef, etc) should be first."

I filed a rietveld lint bug: https://crbug.com/417946.

[wolenetz, 2014-09-26 01:32:08]

On 2014/09/26 01:31:09, wolenetz wrote:
> On 2014/09/26 01:22:31, wolenetz wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
> > File media/base/audio_renderer_mixer.cc (right):
> > 
> >
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
> > media/base/audio_renderer_mixer.cc:61: for (ErrorCallbackList::iterator it =
> > error_callbacks_.begin();
> > I wish for a range-based equivalent here, but we need the iterator for
erase.
> > Any better ideas that the C++11 stuff could give us here?
> > 
> >
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
> > media/base/audio_renderer_mixer.cc:97: for (const auto& cb :
error_callbacks_)
> > aside (I'm filing a crbug): rietveld's lint doesn't like C++11: "Storage
class
> > (static, extern, typedef, etc) should be first."
> > 
> > https://codereview.chromium.org/602193003/diff/1/media/base/audio_splicer.cc
> > File media/base/audio_splicer.cc (right):
> > 
> >
>
https://codereview.chromium.org/602193003/diff/1/media/base/audio_splicer.cc#...
> > media/base/audio_splicer.cc:217: for (const auto& buffer : output_buffers_)
> > aside ditto: (rietveld lint C++11 bug...)
> > "Storage class (static, extern, typedef, etc) should be first."
> 
> I filed a rietveld lint bug: https://crbug.com/417946.

One nit actually: please include "- range-based for loops." in CL description
bullet list.

[Peter Kasting, 2014-09-26 01:52:32]

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

[Peter Kasting, 2014-09-26 01:52:33]

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
media/base/audio_renderer_mixer.cc:61: for (ErrorCallbackList::iterator it =
error_callbacks_.begin();
On 2014/09/26 01:22:31, wolenetz wrote:
> I wish for a range-based equivalent here, but we need the iterator for erase.
> Any better ideas that the C++11 stuff could give us here?

The only thing I can think of is something like this (didn't test this):

  base::AutoLock auto_lock(lock_);
  const auto& i =
      std::find_if(error_callbacks_.begin(), error_callbacks_.end(),
                   std::bind2nd(std::mem_fun(&base::Closure::Equals),
                                error_cb));
  DCHECK(i != error_callbacks_.end());
  error_callbacks_.erase(i);

This isn't actually C++11, and some people are allergic to things like bind2nd
and mem_fun, but to someone comfortable with algorithm it expresses better than
the original code that the purpose here is to find an element matching a
predicate (and assert such an element exists) as opposed to looping over all
elements and performing some kind of operation.

I don't know if there's a better way to do this in C++11, but if there is, it
probably involves lambdas, which aren't OKed yet.

[wolenetz, 2014-09-26 07:23:15]

I'm done perfcrastinating ;)

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
media/base/audio_renderer_mixer.cc:61: for (ErrorCallbackList::iterator it =
error_callbacks_.begin();
On 2014/09/26 01:52:32, Peter Kasting wrote:
> On 2014/09/26 01:22:31, wolenetz wrote:
> > I wish for a range-based equivalent here, but we need the iterator for
erase.
> > Any better ideas that the C++11 stuff could give us here?
> 
> The only thing I can think of is something like this (didn't test this):
> 
>   base::AutoLock auto_lock(lock_);
>   const auto& i =
>       std::find_if(error_callbacks_.begin(), error_callbacks_.end(),
>                    std::bind2nd(std::mem_fun(&base::Closure::Equals),
>                                 error_cb));
>   DCHECK(i != error_callbacks_.end());
>   error_callbacks_.erase(i);
> 
> This isn't actually C++11, and some people are allergic to things like bind2nd
> and mem_fun, but to someone comfortable with algorithm it expresses better
than
> the original code that the purpose here is to find an element matching a
> predicate (and assert such an element exists) as opposed to looping over all
> elements and performing some kind of operation.
> 
> I don't know if there's a better way to do this in C++11, but if there is, it
> probably involves lambdas, which aren't OKed yet.

Bring on the lambdas :). Thanks for the insight - rarely do I see mem_fun and
bind2nd in common code.
The behavior of this alternative would be different vs the original if the
error_cb isn't in error_callbacks_: in the original: NOTREACHED() in release
build would be LOG(ERR, ...), not erase(error_callbacks_.end()). Since not C++11
related, probably doesn't belong in this CL anyway.

[Peter Kasting, 2014-09-26 18:00:14]

On 2014/09/26 07:23:15, wolenetz wrote:
> The behavior of this alternative would be different vs the original if the
> error_cb isn't in error_callbacks_: in the original: NOTREACHED() in release
> build would be LOG(ERR, ...), not erase(error_callbacks_.end()).

NOTREACHED is intended to be a strong guarantee that that case cannot possibly
happen, so that's not a difference that could ever be observed.

[wolenetz, 2014-09-26 19:10:48]

On 2014/09/26 18:00:14, Peter Kasting wrote:
> On 2014/09/26 07:23:15, wolenetz wrote:
> > The behavior of this alternative would be different vs the original if the
> > error_cb isn't in error_callbacks_: in the original: NOTREACHED() in release
> > build would be LOG(ERR, ...), not erase(error_callbacks_.end()).
> 
> NOTREACHED is intended to be a strong guarantee that that case cannot possibly
> happen, so that's not a difference that could ever be observed.

That's true, except if the strong guarantee is broken (which shouldn't happen,
but...). In that case, std::list::erase(invalid position) behavior is undefined,
versus well-defined LOG(ERR,...).

Nit-picking aside, the alternative could easily be adjusted to match the
behavior using a conditional:

    base::AutoLock auto_lock(lock_);
    const auto& i =
    std::find_if(error_callbacks_.begin(), error_callbacks_.end(),
                 std::bind2nd(std::mem_fun(&base::Closure::Equals),
                              error_cb));
    if (i != error_callbacks_end()) {
      error_callbacks_.erase(i);
      return;
    }
    NOTREACHED();

[Peter Kasting, 2014-09-26 20:27:06]

On 2014/09/26 19:10:48, wolenetz wrote:
> On 2014/09/26 18:00:14, Peter Kasting wrote:
> > On 2014/09/26 07:23:15, wolenetz wrote:
> > > The behavior of this alternative would be different vs the original if the
> > > error_cb isn't in error_callbacks_: in the original: NOTREACHED() in
release
> > > build would be LOG(ERR, ...), not erase(error_callbacks_.end()).
> > 
> > NOTREACHED is intended to be a strong guarantee that that case cannot
possibly
> > happen, so that's not a difference that could ever be observed.
> 
> That's true, except if the strong guarantee is broken (which shouldn't happen,
> but...). In that case, std::list::erase(invalid position) behavior is
undefined,
> versus well-defined LOG(ERR,...).

Part of our style guide mandates that we can't put in handler code for DCHECK
failure.  We actually prefer to do things that will crash in this case than
"handle" the problem by logging :)

>     if (i != error_callbacks_end()) {
>       error_callbacks_.erase(i);
>       return;
>     }
>     NOTREACHED();

Right, this would be an example of a construct the style guide would ban.

[wolenetz, 2014-09-26 20:33:15]

On 2014/09/26 20:27:06, Peter Kasting wrote:
> On 2014/09/26 19:10:48, wolenetz wrote:
> > On 2014/09/26 18:00:14, Peter Kasting wrote:
> > > On 2014/09/26 07:23:15, wolenetz wrote:
> > > > The behavior of this alternative would be different vs the original if
the
> > > > error_cb isn't in error_callbacks_: in the original: NOTREACHED() in
> release
> > > > build would be LOG(ERR, ...), not erase(error_callbacks_.end()).
> > > 
> > > NOTREACHED is intended to be a strong guarantee that that case cannot
> possibly
> > > happen, so that's not a difference that could ever be observed.
> > 
> > That's true, except if the strong guarantee is broken (which shouldn't
happen,
> > but...). In that case, std::list::erase(invalid position) behavior is
> undefined,
> > versus well-defined LOG(ERR,...).
> 
> Part of our style guide mandates that we can't put in handler code for DCHECK
> failure.  We actually prefer to do things that will crash in this case than
> "handle" the problem by logging :)
> 
> >     if (i != error_callbacks_end()) {
> >       error_callbacks_.erase(i);
> >       return;
> >     }
> >     NOTREACHED();
> 
> Right, this would be an example of a construct the style guide would ban.

Great points. Thanks for the refresher on DCHECK style :) (If the result of
erase(invalid position) could lead to security issue, the DCHECK should be a
CHECK. In neither case should check failure handler code be included.)

[DaleCurtis, 2014-09-26 20:39:51]

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/602193003/diff/1/media/base/audio_renderer_mi...
media/base/audio_renderer_mixer.cc:61: for (ErrorCallbackList::iterator it =
error_callbacks_.begin();
On 2014/09/26 07:23:15, wolenetz wrote:
> On 2014/09/26 01:52:32, Peter Kasting wrote:
> > On 2014/09/26 01:22:31, wolenetz wrote:
> > > I wish for a range-based equivalent here, but we need the iterator for
> erase.
> > > Any better ideas that the C++11 stuff could give us here?
> > 
> > The only thing I can think of is something like this (didn't test this):
> > 
> >   base::AutoLock auto_lock(lock_);
> >   const auto& i =
> >       std::find_if(error_callbacks_.begin(), error_callbacks_.end(),
> >                    std::bind2nd(std::mem_fun(&base::Closure::Equals),
> >                                 error_cb));
> >   DCHECK(i != error_callbacks_.end());
> >   error_callbacks_.erase(i);
> > 
> > This isn't actually C++11, and some people are allergic to things like
bind2nd
> > and mem_fun, but to someone comfortable with algorithm it expresses better
> than
> > the original code that the purpose here is to find an element matching a
> > predicate (and assert such an element exists) as opposed to looping over all
> > elements and performing some kind of operation.
> > 
> > I don't know if there's a better way to do this in C++11, but if there is,
it
> > probably involves lambdas, which aren't OKed yet.
> 
> Bring on the lambdas :). Thanks for the insight - rarely do I see mem_fun and
> bind2nd in common code.
> The behavior of this alternative would be different vs the original if the
> error_cb isn't in error_callbacks_: in the original: NOTREACHED() in release
> build would be LOG(ERR, ...), not erase(error_callbacks_.end()). Since not
C++11
> related, probably doesn't belong in this CL anyway.

It should never not find the callback, hence the NOTREACHED().

While find_if+erase works, I wouldn't recommend it where only a single element
is expected to be removed. find_if must iterate over the entire container every
time, versus bailing on first match.

Notably we could introduce a templated "zipper" similar to Python's enumerate()
which would zip values such that we could do something like this:

for (const auto& v : base::Enumerate(container)) {
  // v.first == index, v.second == value.
  if (v.second == search_target) {
    container.erase(container.begin() + v.first);
    break;
  }
}

Some googling reveals I'm not the only one with this idea:

http://stackoverflow.com/questions/10962290/find-position-of-element-in-c11-r...

[Peter Kasting, 2014-09-26 20:42:16]

On 2014/09/26 20:39:51, DaleCurtis wrote:
> While find_if+erase works, I wouldn't recommend it where only a single element
> is expected to be removed. find_if must iterate over the entire container
every
> time, versus bailing on first match.

Where do you see that documented?  According to
http://www.cplusplus.com/reference/algorithm/find_if/ , this bails on first
match, which makes sense, since it's only returning an iterator and not a range
(it's not std::remove_if()).

[DaleCurtis, 2014-09-26 20:48:05]

On 2014/09/26 20:42:16, Peter Kasting wrote:
> On 2014/09/26 20:39:51, DaleCurtis wrote:
> > While find_if+erase works, I wouldn't recommend it where only a single
element
> > is expected to be removed. find_if must iterate over the entire container
> every
> > time, versus bailing on first match.
> 
> Where do you see that documented?  According to
> http://www.cplusplus.com/reference/algorithm/find_if/ , this bails on first
> match, which makes sense, since it's only returning an iterator and not a
range
> (it's not std::remove_if()).

Hah, I got that from conflating it with remove_if(). Thanks for correcting me,
it would indeed work fine in this case; though I still prefer the code as
written.

[Peter Kasting, 2014-09-26 20:49:36]

On 2014/09/26 20:48:05, DaleCurtis wrote:
> I still prefer the code as
> written.

Algorithms are pretty ugly pre-lambdas.  I hope when we OK lambdas that people
get more interested in using STL algorithms, because I think after a bit of an
adjustment period they'll be clearer and safer.

[DaleCurtis, 2014-09-26 20:51:17]

On 2014/09/26 20:49:36, Peter Kasting wrote:
> On 2014/09/26 20:48:05, DaleCurtis wrote:
> > I still prefer the code as
> > written.
> 
> Algorithms are pretty ugly pre-lambdas.  I hope when we OK lambdas that people
> get more interested in using STL algorithms, because I think after a bit of an
> adjustment period they'll be clearer and safer.

Yes, I'd prefer the algorithm version w/ lambdas and further plan to clean up
some other in such a manner once they're approved.

[DaleCurtis, 2014-09-26 20:52:21]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-09-26 20:54:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/602193003/1

[commit-bot: I haz the power, 2014-09-26 22:04:53]

Committed patchset #1 (id:1) as 5db6c257041fdfe752807ab468e204704a7490b1

[commit-bot: I haz the power, 2014-09-26 22:05:50]

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