Signing off on this one, commit whenever it fits with everything else. (prior comments about my inability to assess some of this stuff notwithstanding)
On Mon, Oct 27, 2014 at 9:58 AM, Kaelyn Takata <[email protected]> wrote: > And here is the latest version of this patch, with the new test cases > added. > > On Fri, Oct 10, 2014 at 1:30 PM, Kaelyn Takata <[email protected]> wrote: > >> >> >> On Tue, Oct 7, 2014 at 4:24 PM, David Blaikie <[email protected]> wrote: >> >>> >>> >>> On Tue, Oct 7, 2014 at 3:30 PM, Kaelyn Takata <[email protected]> wrote: >>> >>>> On Fri, Oct 3, 2014 at 12:13 PM, David Blaikie <[email protected]> >>>> wrote: >>>> >>>>> MemberTypoDiags looks like it'd be very well-written as a lambda (it's >>>>> just forwarding state to the operator() call). This would imply changing >>>>> the underlying API from unique_ptr<TypoDiagnosticGenerator>, removing >>>>> TypoDiagnosticGenerator, and using std::function<void (const >>>>> TypoCorrection &TC)> instead. The callers would then just pass in lambdas >>>>> and std::function would provide the type erasure, dynamic destruction, >>>>> etc. >>>>> >>>>> MemberExprTypoRecovery is a bit longer, but still does seem like a lambda >>>>> might suite (& even if it doesn't - writing it as a functor (which it >>>>> already is - but you would/could strip off the base class and the >>>>> virtuality of the op()) and having the API use std::function is a bit >>>>> more flexible) >>>>> >>>>> Making MemberExprTypoRecovery a simple functor wouldn't work because >>>> it needs to capture state that isn't passed in when called (the four >>>> arguments to its constructor). >>>> >>> >>> Sorry, perhaps i phrased this poorly - a functor is any object with an >>> operator(). Your TypoDiagnosticGenerator subclasses are already functors. >>> What I'm proposing, more concretely: >>> >>> Remove TypoDiagnosticGenerator >>> Use std::function<void (const TypoCorrection&)> where you had >>> unique_ptr<TypoDiagnosticGenerator> >>> Existing TypoDiagnosticGenerator subclasses can be modified to not >>> inherit from anything, and remove the "override" keyword from operator(). >>> Any existing TypoDiagnosticGenerator subclasses that are simple enough, >>> could be inlined into lambdas (capturing whatever state they need), if >>> desired. >>> >>> So you get the best of both worlds - you can keep the out-of-line/named >>> functors where they're more legible, and use inline lambdas where that's >>> more convenient. >>> >>> (personally I don't mind writing longer functions even like >>> EmptyLookupTypoDiags, and MemberExprTypoRecovery could have the more >>> complex initializations just moved out into a local variable (DeclContext >>> *Ctx = SS.isEmpty() ? nullptr : SemaRef.computeDeclContext(SS, false); ... >>> [=](...) { /* use Ctx here */ })) >>> >>> Anyway, up to you - just a minor change, really. Bit more C++-y. >>> >> >> Ah, I see what you're saying now, and I agree it is a bit more C++-y (not >> to mention a bit more LISPish hehehe). I've converted it over, along with >> converting the functos from the followup patch set. As the proper >> conversion actually touches several of the patches in this set, I haven't >> included the changes here (though they are present in the combined patch I >> mailed you off-list yesterday). >> >>> >>> >>>> As for TypoDiagnosticGenerator, MemberTypoDiags has the same problem of >>>> needing to capture state, and while it is short enough that writing it as a >>>> lambda that captures the needed state wouldn't be too painful, some of the >>>> subsequent TypoDiagnosticGenerator descendants both capture state and are >>>> longer / more complex than MemberTypoDiags (e.g. EmptyLookupTypoDiags in >>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140818/113266.html >>>> is >>>> longer than MemberExprTypoRecovery and has state that goes beyond simply >>>> capturing variables). >>>> >>>>> Otherwise this all looks /fairly/ straightforward (again, I don't have >>>>> all the domain knowledge to judge this - the getDeclFromExpr and its use >>>>> might use comments (and/or the added code that uses it might be pulled >>>>> out into another function with a self-documenting name)). But again, some >>>>> of this is probably just my unfamiliarity with the code. >>>>> >>>>> getDeclFromExpr does exactly what it's name says. :) I've added the >>>> following comment to the one call to getDeclFromExpr to explain what is >>>> being done and why (and added a bit of whitespace around the code block, >>>> which had been moved to the EmitAllDiagnostics helper added in response to >>>> your comments on patch #5): >>>> >>>> // Extract the NamedDecl from the transformed TypoExpr and add >>>> it to the >>>> // TypoCorrection, replacing the existing decls. This ensures >>>> the right >>>> // NamedDecl is used in diagnostics e.g. in the case where >>>> overload >>>> // resolution was used to select one from several possible >>>> decls that >>>> // had been stored in the TypoCorrection. >>>> >>> >>> Ah, I guess that's the kicker - overload resolution, etc. Thanks muchly. >>> >> >> You're welcome. :) >> >>> >>> >>>> if (auto *ND = getDeclFromExpr( >>>> Replacement.isInvalid() ? nullptr : Replacement.get())) >>>> TC.setCorrectionDecl(ND); >>>> >>>> >>>>> On Thu, Sep 25, 2014 at 2:00 PM, Kaelyn Takata <[email protected]> >>>>> wrote: >>>>> >>>>>> ping >>>>>> >>>>>> On Tue, Aug 26, 2014 at 11:06 AM, Kaelyn Takata <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> +dblaikie >>>>>>> >>>>>>> >>>>>>> On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> >>>>>>>> This includes adding the new TypoExpr-based lazy typo correction to >>>>>>>> LookupMemberExprInRecord as an alternative to the existing eager >>>>>>>> typo >>>>>>>> correction. >>>>>>>> --- >>>>>>>> lib/Sema/SemaExprCXX.cpp | 40 ++++++++++- >>>>>>>> lib/Sema/SemaExprMember.cpp | 116 >>>>>>>> ++++++++++++++++++++++++++++--- >>>>>>>> test/SemaCXX/arrow-operator.cpp | 5 +- >>>>>>>> test/SemaCXX/typo-correction-delayed.cpp | 32 +++++++++ >>>>>>>> test/SemaCXX/typo-correction-pt2.cpp | 2 +- >>>>>>>> test/SemaCXX/typo-correction.cpp | 10 +-- >>>>>>>> 6 files changed, 186 insertions(+), 19 deletions(-) >>>>>>>> create mode 100644 test/SemaCXX/typo-correction-delayed.cpp >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
