Doug,<br><br>Thanks for the comments! I've attached an updated patch that includes a few small changes based on your feedback.<br><br><div class="gmail_quote">On Wed, Aug 3, 2011 at 10:29 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div><div></div><div class="h5"><br><div><div>On Aug 1, 2011, at 4:25 PM, Kaelyn Uhrain wrote:</div>
<br><blockquote type="cite"><div class="gmail_quote">On Mon, Aug 1, 2011 at 3:54 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@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 class="gmail_quote"><div>On Thu, Jul 28, 2011 at 2:25 PM, Kaelyn Uhrain <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>Here's an updated version of my patch to add function overload resolution to the typo correction, eliminating one of the ways that Sema::CorrectTypo returns a non-keyword correction without a corresponding NamedDecl. It incorporates all of the changes and feedback from <a href="http://codereview.appspot.com/4747047" target="_blank">http://codereview.appspot.com/4747047</a>. Given it's been almost two weeks since I sent out the first version and over a week since I made changes based on Chandler's last feedback (I was hoping he'd get a chance to look at those changes, but he's been swamped), I'd appreciate it of someone could give it a once-over and let me know whether it is ready for submission.<br>
</div></blockquote><div><br></div></div><div>I've made a few additional comments, but my primary concern with this patch is the representation of KeywordDecl. I think playing fast and loose with magical pointer values is going to come back to bite us. If we can't use 'NULL' as the magical pointer value, we should find some more robust way to represent a keyword in the correction results.</div>
<div><br></div><div>I'd certainly appreciate ideas from dgregor and any others on the list on the best way to represent that.</div><div><br></div><div>To make it easier for others to review, can you attach the latest patch to the email thread?</div>
</div>
</blockquote></div><br>The use of a special non-NULL value for the CorrectionDecl* was needed when the TypoCorrection stored just one NamedDecl pointer to avoid an extra internal boolean flag and a bunch of handling for it, but is unneeded now that TypoCorrection stores a list of NamedDecl pointers (can now do empty list vs NULL first element). I've attached the updated patch for real this time--sorry for accidentally sending my previous email with a missing attachment.<br>
</blockquote></div><br></div></div><div>A few comments…</div><div><br></div><div><br></div><div><div>@@ -1419,6 +1420,27 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,</div><div> R.setLookupName(Corrected.getCorrection());</div>
<div> </div><div> if (NamedDecl *ND = Corrected.getCorrectionDecl()) {</div><div>+ if (Args && Corrected.isOverloaded()) {</div><div><br></div><div>One could actually have zero arguments </div></div></div>
</blockquote><div><br>The Args argument is the same one passed to BuildRecoveryCallExpr (the other caller of DiagnoseEmptyLookup passes in NULL); since it's a pointer to a pointer and presumably BuildRecoveryCallExpr handles zero argument calls to, I'd thought Args would be a non-NULL pointer even if NumArgs is zero. I'm guessing from your comment I was mistaken about that.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><br></div><div>+ OverloadCandidateSet OCS(R.getNameLoc());</div>
<div>+ OverloadCandidateSet::iterator Best;</div><div>+ for (TypoCorrection::decl_iterator CD = Corrected.begin(),</div><div>+ CDEnd = Corrected.end();</div><div>+ CD != CDEnd; ++CD) {</div>
<div>+ assert(isa<FunctionDecl>(*CD) &&</div><div>+ "Encountered a non-function as a function overload candidate");</div><div>+ AddOverloadCandidate(cast<FunctionDecl>(*CD),</div>
<div>+ DeclAccessPair::make(*CD, AS_none),</div><div>+ Args, NumArgs, OCS);</div><div>+ }</div><div><br></div><div>The declarations we see here aren't necessarily all FunctionDecls. We may also see FunctionTemplateDecls, which have a different Add*Candidate call (that also does template argument deduction). Plus, we could see using declarations (which we'd need to look through) or unresolved using declarations (which we'd probably just give up on).</div>
<div><br></div><div>Also, the FunctionDecls and FunctionTemplateDecls may be for member functions, so we'd need a "this" argument to pass along to the Add*Candidate functions used when calling a member function.</div>
</div></div></blockquote><div><br>I don't think that is necessary, judging by the comments in Sema::AddOverloadCandidate around line 3086 of SemaOverload.cpp (and the fact it just calls AddMethodCandidate in the non-contructor CXXMethodDecl case). <br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div><br></div><div>You don't need to handle all of these cases in a first patch, but right now it looks like it's going to introduce a crash when it tries to test overloading when correcting to a FunctionTemplateDecl. I see that your test case doesn't actually cover this, since "fin" gets corrected to the non-templated functions "min".</div>
</div></div></blockquote><div><br>I've removed the assertion, changed the cast<> to a dyn_cast<> in a conditional, and added a TODO for handling other overloaded Decl types besides FunctionDecl and its derivatives.<br>
<br>Thanks,<br>Kaelyn<br></div></div><br>