<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 7, 2014 at 3:30 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Fri, Oct 3, 2014 at 12:13 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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 (<span style="color:rgb(0,0,0)">const TypoCorrection &TC)> instead. The callers would then just pass in lambdas and std::function would provide the type erasure, dynamic destruction, etc.<br><br></span><pre style="color:rgb(0,0,0)">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)</pre></div></blockquote></span><div>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). </div></div></div></div></blockquote><div><br></div><div>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:<br><br>Remove TypoDiagnosticGenerator<br>Use std::function<void (const TypoCorrection&)> where you had unique_ptr<TypoDiagnosticGenerator><br>Existing TypoDiagnosticGenerator subclasses can be modified to not inherit from anything, and remove the "override" keyword from operator().<br>Any existing TypoDiagnosticGenerator subclasses that are simple enough, could be inlined into lambdas (capturing whatever state they need), if desired.<br><br>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.<br><br>(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 */ }))<br><br>Anyway, up to you - just a minor change, really. Bit more C++-y.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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 <a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140818/113266.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140818/113266.html</a> is longer than MemberExprTypoRecovery and has state that goes beyond simply capturing variables).<br></div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)">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.</pre></div></blockquote></span><div>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):</div><div><br></div><div><div> // Extract the NamedDecl from the transformed TypoExpr and add it to the</div><div> // TypoCorrection, replacing the existing decls. This ensures the right</div><div> // NamedDecl is used in diagnostics e.g. in the case where overload</div><div> // resolution was used to select one from several possible decls that</div><div> // had been stored in the TypoCorrection.</div></div></div></div></div></blockquote><div><br></div><div>Ah, I guess that's the kicker - overload resolution, etc. Thanks muchly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div> if (auto *ND = getDeclFromExpr(</div><div> Replacement.isInvalid() ? nullptr : Replacement.get()))</div><div> TC.setCorrectionDecl(ND);</div></div><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><pre style="color:rgb(0,0,0)"></pre></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 25, 2014 at 2:00 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">ping</div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 11:06 AM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">+dblaikie</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
This includes adding the new TypoExpr-based lazy typo correction to<br>
LookupMemberExprInRecord as an alternative to the existing eager typo<br>
correction.<br>
---<br>
lib/Sema/SemaExprCXX.cpp | 40 ++++++++++-<br>
lib/Sema/SemaExprMember.cpp | 116 ++++++++++++++++++++++++++++---<br>
test/SemaCXX/arrow-operator.cpp | 5 +-<br>
test/SemaCXX/typo-correction-delayed.cpp | 32 +++++++++<br>
test/SemaCXX/typo-correction-pt2.cpp | 2 +-<br>
test/SemaCXX/typo-correction.cpp | 10 +--<br>
6 files changed, 186 insertions(+), 19 deletions(-)<br>
create mode 100644 test/SemaCXX/typo-correction-delayed.cpp<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>