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

[Kaelyn Takata]

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 <rikka at google.com> wrote:

>
>
> 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/20141027/cba87067/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v5-0008-Wire-up-LookupMemberExpr-to-use-the-new-TypoExpr.patch
Type: text/x-patch
Size: 14863 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141027/cba87067/attachment.bin>