1339924 - Build error with clang trunk: dom/media/gtest/TestMozPromise.cpp:198:8: error: lambda capture 'queue' is not used [-Werror,-Wunused-lambda-capture]

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Daniel Holbert [:dholbert]]

When I build mozilla-central with clang trunk (llvm revision 295219) and
ac_add_options --enable-warnings-as-errors, I'm blocked at the following build
warning treated as an error:

=====
dom/media/gtest/TestMozPromise.cpp:198:8: error: lambda capture 'queue' is not used [-Werror,-Wunused-lambda-capture]

       [queue] (int aVal) -> RefPtr<TestPromise> { return TestPromise::CreateAndReject(double(aVal - 42) + 42.0, __func__); },
====

(It appears this build warning "-Wunused-lambda-capture" is a recently-added clang feature.)

Bobby, do you have any idea what's going on here? Any suggestions for fixing?

Comment 1

[Daniel Holbert [:dholbert]]

Here's the code in question. The clang warning is pointing to the second-to-last line here:

>  RefPtr<TaskQueue> queue = atq.Queue();
>  RunOnTaskQueue(queue, [queue, &invokedPass] () -> void {
>    TestPromise::CreateAndResolve(40, __func__)
>    ->Then(queue, __func__,
>      [] (int aVal) -> RefPtr<TestPromise> { return TestPromise::CreateAndResolve(aVal + 10, __func__); },
>      DO_FAIL)
>    ->Then(queue, __func__, [&invokedPass] () -> void { invokedPass = true; }, DO_FAIL)
>    ->Then(queue, __func__,
>      [queue] (int aVal) -> RefPtr<TestPromise> {
>        RefPtr<TestPromise::Private> p = new TestPromise::Private(__func__);
>        nsCOMPtr<nsIRunnable> resolver = new DelayedResolveOrReject(queue, p, RRValue::MakeResolve(aVal - 8), 10);
>        queue->Dispatch(resolver.forget());
>        return RefPtr<TestPromise>(p);
>      },
>      DO_FAIL)
>    ->Then(queue, __func__,
>      [queue] (int aVal) -> RefPtr<TestPromise> { return TestPromise::CreateAndReject(double(aVal - 42) + 42.0, __func__); },
>      DO_FAIL)
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/media/gtest/TestMozPromise.cpp#198

Comment 2

[Daniel Holbert [:dholbert]]

(CC'ing jwwang & gerald who may've been looking at this code more recently than bholley, based on hg log)

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I'm guessing the lambda was copied from above, with the same capture `[queue]`, though it is unneeded in this case as there is no dispatch in the lambda body.

Happy to remove it, will take the bug in ~1h unless someone else beats me to it.

Comment 4

[Daniel Holbert [:dholbert]]

Awesome, thanks!

Comment 5

[Daniel Holbert [:dholbert]]

Actually, maybe we don't want to change this...  "queue" is a RefPtr, and perhaps the test is intending to keep |queue| alive until the Lambda completes? (Though maybe it doesn't need to.)

Bug 1339933 is similar and I suspect we *don't* want to remove the lambda capture there. Not as sure about this case though. (depends on if/how the queue is used indirectly somehow)

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Good thinking!
However in this particular case, there is a subsequent lambda that also captures the queue, and that 2nd lambda is in a Then() and therefore will always stay alive longer than the first one. So it should be safe to remove the captured queue from the first lambda.
Or we can just mark it `Unused <<` to be extra safe.
I'll have a deeper look and see what's best...


Regarding bug 1339933, indeed the captured RefPtr is needed to keep 'this' alive, but the RefPtr itself is not used inside the lambda.
I've seen this pattern used in many places, so maybe the warning needs tweaking -- i.e., we write the equivalent static checker that ignores the this+RefPtr(this) pattern.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Also also, the lambda at fault is inside a Then(queue, ...) which itself keeps a reference to the queue, because that's where the MozPromise code will run the given lambda, so we are doubly-protected!

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1339924 - Remove unneeded [queue] lambda capture -

'queue' is not used inside that lambda, so we can drop it.

Another option would have been to simulate its use through `Unused <<`.
But I think that it's a useful test, *not* to capture the queue here, to verify
that it is correctly captured in the enclosing MozPromise::Then.

Review commit: https://reviewboard.mozilla.org/r/112826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112826/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8837834 [details]
Bug 1339924 - Remove unneeded [queue] lambda capture -

https://reviewboard.mozilla.org/r/112826/#review114350

Looks good to me!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9137f214b994
Remove unneeded [queue] lambda capture - r=dholbert

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9137f214b994