<div dir="ltr">Signing off on this one, commit whenever it fits with everything else. (prior comments about my inability to assess some of this stuff notwithstanding)</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 27, 2014 at 9:58 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">And here is the latest version of this patch, with the new test cases added.</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 10, 2014 at 1: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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Oct 7, 2014 at 4:24 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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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: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"><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></span><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></div></blockquote><div><br></div></span><div>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).</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"><div class="gmail_extra"><div class="gmail_quote"><span><div> </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"><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" rel="noreferrer" 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></span><div>Ah, I guess that's the kicker - overload resolution, etc. Thanks muchly.</div></div></div></div></blockquote><div><br></div></span><div>You're welcome. :) </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"><div class="gmail_extra"><div class="gmail_quote"><span><div> </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"><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></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>