Closed Bug 1339924 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: dholbert, Assigned: mozbugz)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(bobbyholley)
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
(CC'ing jwwang & gerald who may've been looking at this code more recently than bholley, based on hg log)
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.
Awesome, thanks!
Assignee: nobody → gsquelart
Flags: needinfo?(bobbyholley)
See Also: → 1339933
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)
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.
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 on attachment 8837834 [details]
Bug 1339924 - Remove unneeded [queue] lambda capture -

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

Looks good to me!
Attachment #8837834 - Flags: review?(dholbert) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9137f214b994
Remove unneeded [queue] lambda capture - r=dholbert
https://hg.mozilla.org/mozilla-central/rev/9137f214b994
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: