[PATCH v4 8/8] Wire up LookupMemberExpr to use the new TypoExpr.

[Kaelyn Takata]

On Tue, Oct 7, 2014 at 4:24 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Tue, Oct 7, 2014 at 3:30 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>> On Fri, Oct 3, 2014 at 12:13 PM, David Blaikie <dblaikie at gmail.com>
>> 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 <rikka at google.com> wrote:
>>>
>>>> ping
>>>>
>>>> On Tue, Aug 26, 2014 at 11:06 AM, Kaelyn Takata <rikka at google.com>
>>>> wrote:
>>>>
>>>>> +dblaikie
>>>>>
>>>>>
>>>>> On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <rikka at google.com>
>>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141010/0d8f287c/attachment.html>