Closed Bug 1322962 Opened 8 years ago Closed 7 years ago

Allow capture of UniquePtr by C++11 lambdas

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: mozbugz, Assigned: mozbugz)

Details

Attachments

(2 files)

Bug 1322962 - UniquePtrForCpp11Lambda -
58 bytes, text/x-review-board-request
Details
Bug 1322962 - UniquePtrForCpp11Lambda static checker -
59 bytes, text/x-review-board-request
ehsan.akhgari
: review-
Details 
C++11 lambdas can only capture variables by-value, by making copies.
C++14 will add custom-initialized captures, which will allow moving an object into a lambda.
Until then, this C++11 limitation prevents the use of UniquePtr in lambdas.

The idea is to have a simple UniquePtr wrapper, so we can start using UniquePtr in lambdas.
Later we can easily change these wrappers into real UniquePtr's.
Also this wrapper will allow back-porting future UniquePtr-in-lambda code to previous C++11-only versions.

One use of this wrapper will be demonstrated in a follow-up bug.
Component: XPCOM → MFBT
Why not just require that lambdas capture UniquePtr by reference?  That seems pretty simple and adequate to me, at least on its face.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Why not just require that lambdas capture UniquePtr by reference?  That
> seems pretty simple and adequate to me, at least on its face.

The use-cases where I'm first going to use this store and later dispatch lambdas, so the initial scope with the referenced UniquePtr would most likely be gone by the time the lambda runs.
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

https://reviewboard.mozilla.org/r/98156/#review98458

::: mfbt/UniquePtr.h:758
(Diff revision 1)
> +  void operator=(const UniquePtrForCpp11Lambda& aOther) = delete;
> +
> +  // Most common UniquePtr-like methods.
> +  T* get() const { return mUPtr.get(); }
> +  explicit operator bool() const { return get(); }
> +  T& operator*() const { return *get(); }

We should also assert the get() here like line 761 did. Maybe UniquePtr implementation should do that check also.
(In reply to Gerald Squelart [:gerald] from comment #3)
> The use-cases where I'm first going to use this store and later dispatch
> lambdas

Ah, that's sensible.

As to the implementation...I need to think about this, but I'm initially leery.  This isn't all that different from giving UniquePtr a copy constructor that silently nulls out the provided argument -- which is basically why std::auto_ptr is now deprecated and thus is a solid argument against this.

Moreover, C++14 initialized lambda captures are coming the next time we update our GCC compiler version requirements, assuming that's the name for this:

https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

With that in mind (and keeping in mind I still need to think about the implementation more, so this isn't a final answer): I'm somewhat inclined to say we should just wait for the GCC version bump, and in the meantime eschew capturing UniquePtr -- maybe handwriting the closure class in those rare cases.  But, comments from others appreciated.
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

https://reviewboard.mozilla.org/r/98156/#review98480

::: mfbt/UniquePtr.h:747
(Diff revision 1)
> +  {}
> +#else
> +  // For some reason, MSC doesn't see the non-const-ref copy constructor above
> +  // when lambda-capturing, so here's an ugly hack that works.
> +  // It assumes that the given UniquePtr is not really const.
> +  UniquePtrForCpp11Lambda(const UniquePtrForCpp11Lambda& aOther)

Maybe we can mark mUPtr as "mutable" and only provide a const version of copy constructor for workaround.
Thank you for your thoughtful comments, Jeff, much appreciated.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> (In reply to Gerald Squelart [:gerald] from comment #3)
> > The use-cases where I'm first going to use this store and later dispatch
> > lambdas
> 
> Ah, that's sensible.

Thanks. You can see my first attempt at using it there:
  https://reviewboard.mozilla.org/r/98252/diff/1#index_header

> As to the implementation...I need to think about this, but I'm initially
> leery.  This isn't all that different from giving UniquePtr a copy
> constructor that silently nulls out the provided argument -- which is
> basically why std::auto_ptr is now deprecated and thus is a solid argument
> against this.

Totally agree with nsAutoPtr/auto_ptr being deprecated, and the leeriness of using a copy constructor in what should be move-only!

Some of my reasons for creating this new wrapper that's quite close to nsAutoPtr:
- The name "UniquePtrForCpp11Lambda" makes it clear that the intent is to use it only for UniquePtr-in-C++11-lambda cases. (We could probably even write a static checker for that?)
- We can start writing safer UniquePtr-in-lambda now, instead of using nsAutoPtr or raw pointers.
- It should be an easy task to find them and convert them to proper move-only code when C++14 becomes available.
- If future code needs to be backported to a C++11-only release, we can backport UniquePtrForCpp11Lambda with it.

> Moreover, C++14 initialized lambda captures are coming the next time we
> update our GCC compiler version requirements, assuming that's the name for
> this:
> https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

I actually discussed this with Nathan and Mike last week, and I'm also hopeful that C++14 might arrive sooner than I anticipated, see bug 1322792.

I'm still keen on making some existing (or soon-to-be-written) code a bit safer in the meantime, because I've been burnt waiting for other things to happen, so I just wanted to go for this ASAP anyway, in case C++14 doesn't make it soon-ish.

> With that in mind (and keeping in mind I still need to think about the
> implementation more, so this isn't a final answer): I'm somewhat inclined to
> say we should just wait for the GCC version bump, and in the meantime eschew
> capturing UniquePtr -- maybe handwriting the closure class in those rare
> cases.  But, comments from others appreciated.

Yes, happy to gather more comments&thoughts, and even to desist if people think it's best to just wait for C++14...
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

https://reviewboard.mozilla.org/r/98156/#review99092

This review was delayed longer than necessary. :(

I'm inclined to say we should wait for C++14 at this point.  While `UniquePtrForCpp11Lambda` does make the code better in the proposed patch elsewhere, I don't know that the code is improved so much that we simply must have `UniquePtrForCpp11Lambda`.  And the same applies for other use cases of `UniquePtrForCpp11Lambda`: at worst, we'd have to `.release()` and reacquire the `UniquePtr` in the lambda, which is...probably OK?

If we get to Firefox 55 and C++14 hasn't landed, we could reconsider.
Attachment #8817978 - Flags: review?(nfroyd)
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

https://reviewboard.mozilla.org/r/98156/#review99092

To be thorough:
> we'd have to `.release()` and reacquire the `UniquePtr` in the lambda
I'm afraid it's not that simple: A `MozPromise::Then()` request may be `Disconnect()`ed, meaning the lambda may not always run, leading to a leak if a raw pointer was captured.
We really want the lambda to own the pointer in its capture.
Sorry to insist, but I do think something needs to be done, if not my proposed solution...

Please see comment 9 (we really want to capture an owning pointer).

55 is almost four months away (hopefully it will be shorter of course). I'd be uncomfortable leaving this potential for leaks, and to prevent new safer uses for this long. And we'd still have the issue of backporting C++14 code to C++11-only releases.

If you'd prefer not to introduce this new class UniquePtrForCpp11Lambda, I could just use nsAutoPtr instead (with a small fix for MSVC) -- the devil we know... WDYT?
Flags: needinfo?(nfroyd)
(In reply to Gerald Squelart [:gerald] from comment #10)
> Sorry to insist, but I do think something needs to be done, if not my
> proposed solution...
> 
> Please see comment 9 (we really want to capture an owning pointer).
> 
> 55 is almost four months away (hopefully it will be shorter of course). I'd
> be uncomfortable leaving this potential for leaks, and to prevent new safer
> uses for this long. And we'd still have the issue of backporting C++14 code
> to C++11-only releases.
> 
> If you'd prefer not to introduce this new class UniquePtrForCpp11Lambda, I
> could just use nsAutoPtr instead (with a small fix for MSVC) -- the devil we
> know... WDYT?

Thanks for pointing that out.  Handling this safely is a reasonable thing to want.

Why don't we go ahead and land this; I'd really rather not encourage people to use nsAutoPtr at this point, and the name makes it clearer what you should be doing with it.

We should be upgrading to GCC 4.9 shortly, and then C++14 in the very late 53 or early-ish 54 timeline; I'm unclear why we'd have to wait until 55.  But maybe having this in the tree will make backporting easier...
Flags: needinfo?(nfroyd)
Looks like there is good progress on the C++14 front (bug 1322792, bug 1325632), so I'll hold off on this one, hopefully it won't be needed at all!
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

(forcing r?)
Attachment #8817978 - Flags: review?(nfroyd)
I cannot see a way to revive the 'r?' in mozreview. :-/
Nathan, could you please re-review?
Flags: needinfo?(nfroyd)
(In reply to Gerald Squelart [:gerald] from comment #17)
> I cannot see a way to revive the 'r?' in mozreview. :-/
> Nathan, could you please re-review?

The request came through in bugzilla; I don't know why mozreview refuses to show you the right thing. :(
Flags: needinfo?(nfroyd)
Thank you.

Also I see that you are progressing on making C++14 usable (bug 1325632), so I'm happy to hold off landing for a couple of weeks, in case C++14 sticks and I can use it in bug 1322964.
Try, with one proper use of UniquePtrForCpp11Lambda (from dependent bug 1322964), which was correctly allowed by the checker:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32888e6359275bd4cecaf4dd9c95bb337dcdae43
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

https://reviewboard.mozilla.org/r/98156/#review103502

::: mfbt/UniquePtr.h:740
(Diff revision 5)
> +  }
> +
> +  // The copy-constructor that shouldn't exist, which makes capturing by a
> +  // C++11 lambda possible.
> +  UniquePtrForCpp11Lambda(
> +#ifdef _MSC_VER

Please only add this hack for MSVC and not clang-cl.
Comment on attachment 8824374 [details]
Bug 1322962 - UniquePtrForCpp11Lambda static checker -

https://reviewboard.mozilla.org/r/102898/#review103498

For the record, I agree with the others that this is probably not worth doing, and instead we should just wait until we switch to C++14.  I unfortunately only saw this bug for the first time here, so I don't want to r- because of this reason now that you've done all of this work...  Minusing for the comments below though.

::: build/clang-plugin/Checks.inc:30
(Diff revision 2)
>  CHECK(RefCountedCopyConstructorChecker, "refcounted-copy-constructor")
>  CHECK(RefCountedInsideLambdaChecker, "refcounted-inside-lambda")
>  CHECK(ScopeChecker, "scope")
>  CHECK(SprintfLiteralChecker, "sprintf-literal")
>  CHECK(TrivialCtorDtorChecker, "trivial-constructor-destructor")
> +CHECK(UniquePtrForCpp11LambdaChecker, "UniquePtrForCpp11LambdaChecker-checker")

We name the static checks around the thing that they actually check, not the Mozilla types they currently apply to.  So a better name for this checker would be NoCopyUnlessInLambdaCaptureChecker.  Also, please stick to the existing convention for the human readable name.

::: build/clang-plugin/UniquePtrForCpp11LambdaChecker.cpp:21
(Diff revision 2)
> +          hasDeclaration(
> +            cxxConstructorDecl(
> +              isCopyConstructor(),
> +              hasParent(
> +                classTemplateSpecializationDecl(
> +                  hasName("mozilla::UniquePtrForCpp11Lambda")

We don't usually special case class names like this.  Instead, we rely on Gecko annotating classes with attributes that we recognize here using custom matchers that call hasCustomAnnotation.  See CustomMatchers.h for examples of the matchers, and mfbt/Attributes.h for the examples of the attributes we declare.

::: build/clang-plugin/UniquePtrForCpp11LambdaChecker.cpp:36
(Diff revision 2)
> +  // UniquePtrForCpp11Lambda copy under non-lambda Expr is always bad.
> +  AstMatcher->addMatcher(
> +    expr(
> +      unless(lambdaExpr()),
> +      has(
> +        cxxConstructExpr(

You don't need two matchers here.  I think you can only have one which matches constructor expressions that invoke this copy ctor, and then look at the parent in check() and decide whether to diagnose accordingly.

::: build/clang-plugin/tests/TestUniquePtrForCpp11LambdaChecker.cpp:16
(Diff revision 2)
> +
> +  // The copy-constructor that shouldn't exist, which makes capturing by a
> +  // C++11 lambda possible.
> +  UniquePtrForCpp11Lambda(
> +#ifdef _MSC_VER
> +  // For some reason, MSC doesn't see the non-const-ref copy constructor

Why are you adding an MSVC hack to a file that MSVC never even looks at?

These tests are *only* run with clang(-cl).
Attachment #8824374 - Flags: review?(ehsan) → review-
Thank you for the feedback everybody.

I'll count Ehsan's "not worth doing" as a third strike against, so I'll just close this bug and (eagerly) wait for C++14...

At least I've learned how to write static checkers. :-)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
(In reply to Gerald Squelart [:gerald] from comment #26)
> Thank you for the feedback everybody.
> 
> I'll count Ehsan's "not worth doing" as a third strike against, so I'll just
> close this bug and (eagerly) wait for C++14...

Sorry you had to throw out your work...  I hate it when it happens.  :(

> At least I've learned how to write static checkers. :-)

Yes!  Your check looked really good, FWIW!  If you can ever think of a problematic pattern that can be checked like this, please let me know, especially if you needed help.
Comment on attachment 8817978 [details]
Bug 1322962 - UniquePtrForCpp11Lambda -

Canceling this review, then.
Attachment #8817978 - Flags: review?(nfroyd)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: