[PATCH 3/8] Pass around CorrectionCandidateCallbacks as unique_ptrs so TypoCorrectionConsumer can keep the callback around as long as needed.

[Kaelyn Takata]

On Tue, Jun 17, 2014 at 2:44 PM, David Blaikie <dblaikie at gmail.com> wrote:

> (things might be easier to review in Phab, but I'm not sure how easy
> it is to manage a series of dependent patches is there)
>

Yup, that it's a series of dependent patches instead of one enormous patch
is why I decided this wasn't the time for me to try out Phabricator.


> TypoCorrectionConsumer's ctor:
>
> "std::unique_ptr<CorrectionCandidateCallback> &CCC," - looks like
> you're actually trying to pass ownership here. Pass the unique_ptr by
> value to represent this contract.
>
> CorrectTypo function:
>
> "std::unique_ptr<CorrectionCandidateCallback> &&CCC," - also passing
> ownership. Passing rvalue ref is certainly more helpful than passing
> by const ref, but passing by value is potentially even better. When
> passing by value it's unambiguous that the caller will take ownership
> and that the callee's unique_ptr will be null. With an rvalue
> reference it's still possible for a caller to not take ownership, or
> to put some other value in the unique_ptr that the callee is meant to
> use... a bad idea, but possible.
>
> Same for ClassifyName's "CCC" parameter.
>

The unique_ptr reference (both & and &&) parameters were because I was
getting multiple compile errors. I just went through and made all of the
cases pass-by-value and with the current version of the patches and only
got one compile error, which was solved with a std::move(). I actually
meant to try to attempt switching the unique_ptrs back to pass-by-value
before mailing the patches since the patches have been changed considerably
since I originally saw the errors (oh the dangers of a long weekend! lol).


>
>
> 2014-06-17 14:14 GMT-07:00 Kaelyn Takata <rikka at google.com>:
> > ---
> >  include/clang/Parse/Parser.h      |  5 ++--
> >  include/clang/Sema/Sema.h         | 27 +++++++++----------
> >  include/clang/Sema/SemaInternal.h | 18 ++++++-------
> >  lib/Parse/ParseExpr.cpp           | 10 +++----
> >  lib/Parse/ParseStmt.cpp           |  6 ++---
> >  lib/Parse/ParseTentative.cpp      |  8 +++---
> >  lib/Parse/Parser.cpp              |  8 +++---
> >  lib/Sema/SemaCXXScopeSpec.cpp     |  9 +++----
> >  lib/Sema/SemaDecl.cpp             | 57
> +++++++++++++++++++--------------------
> >  lib/Sema/SemaDeclCXX.cpp          | 27 ++++++++++---------
> >  lib/Sema/SemaDeclObjC.cpp         | 21 +++++++--------
> >  lib/Sema/SemaExpr.cpp             | 35 ++++++++++++------------
> >  lib/Sema/SemaExprMember.cpp       | 15 +++++------
> >  lib/Sema/SemaExprObjC.cpp         | 18 ++++++-------
> >  lib/Sema/SemaInit.cpp             |  4 +--
> >  lib/Sema/SemaLambda.cpp           |  4 +--
> >  lib/Sema/SemaLookup.cpp           | 21 ++++++++-------
> >  lib/Sema/SemaOpenMP.cpp           |  7 +++--
> >  lib/Sema/SemaOverload.cpp         | 23 +++++++++-------
> >  lib/Sema/SemaTemplate.cpp         | 17 ++++++------
> >  lib/Sema/SemaTemplateVariadic.cpp |  8 +++---
> >  21 files changed, 175 insertions(+), 173 deletions(-)
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140617/c655a71b/attachment.html>