Bug 127274 – Move WebAudio source code to std::unique_ptr

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

Move WebAudio source code to std::unique_ptr

[Zan Dobersek]

Created attachment 221637 [details]
Patch

[Zan Dobersek]

Comment on attachment 221637 [details]
Patch

Clearing flags on attachment: 221637

Committed r162368: <http://trac.webkit.org/changeset/162368>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 221637 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221637&action=review

> Source/WebCore/Modules/webaudio/AudioNode.cpp:334
> -    ASSERT(m_inputs.contains(input));
> -    if (!m_inputs.contains(input))
> -        return;
> +    for (const std::unique_ptr<AudioNodeInput>& savedInput : m_inputs) {
> +        if (input == savedInput.get()) {
> +            input->updateInternalBus();
> +            return;
> +        }
> +    }
>  
> -    input->updateInternalBus();
> +    ASSERT_NOT_REACHED();

The new code seems clearly worse than the old here. Is there something we can do about that? Maybe we can overload the contains function so it can be used with a raw pointer in a case like this?

> Source/WebCore/platform/audio/HRTFDatabaseLoader.cpp:80
> -    m_hrtfDatabase.clear();
> +    m_hrtfDatabase.reset();

Normally we just use

    m_hrtfDatabase = nullptr;

for a case like this.

> Source/WebCore/platform/audio/HRTFKernel.h:60
> +        return adoptRef(new HRTFKernel(std::forward<std::unique_ptr<FFTFrame>>(fftFrame), frameDelay, sampleRate));

I’m really surprised that we needed the template argument for std::forward here. I would expect just std::forward(fftFrame) or std::move(fftFrame) would work.

[Anders Carlsson]

(In reply to comment #4)
> The new code seems clearly worse than the old here. Is there something we can do about that? Maybe we can overload the contains function so it can be used with a raw pointer in a case like this?

I'd actually like to get rid of Vector::contains since it's O(n). Forcing users to write out the full loop makes it more obvious that it's a linear operation.

> 
> > Source/WebCore/platform/audio/HRTFDatabaseLoader.cpp:80
> > -    m_hrtfDatabase.clear();
> > +    m_hrtfDatabase.reset();
> 
> Normally we just use
> 
>     m_hrtfDatabase = nullptr;
> 
> for a case like this.
> 
> > Source/WebCore/platform/audio/HRTFKernel.h:60
> > +        return adoptRef(new HRTFKernel(std::forward<std::unique_ptr<FFTFrame>>(fftFrame), frameDelay, sampleRate));
> 
> I’m really surprised that we needed the template argument for std::forward here. I would expect just std::forward(fftFrame) or std::move(fftFrame) would work.

std::forward should only be used together with template type parameters. This should use move.