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

[David Blaikie]

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 <rikka at google.com> 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 <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/36e7b320/attachment.html>