[cfe-commits] PATCH: Better handle overloaded functions in typo correction

[Douglas Gregor]

On Aug 1, 2011, at 4:25 PM, Kaelyn Uhrain wrote:

> On Mon, Aug 1, 2011 at 3:54 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Thu, Jul 28, 2011 at 2:25 PM, Kaelyn Uhrain <rikka at google.com> wrote:
> 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 http://codereview.appspot.com/4747047. 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.
> 
> 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.
> 
> I'd certainly appreciate ideas from dgregor and any others on the list on the best way to represent that.
> 
> To make it easier for others to review, can you attach the latest patch to the email thread?
> 
> 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.

A few comments…


@@ -1419,6 +1420,27 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
     R.setLookupName(Corrected.getCorrection());
 
     if (NamedDecl *ND = Corrected.getCorrectionDecl()) {
+      if (Args && Corrected.isOverloaded()) {

One could actually have zero arguments 

+        OverloadCandidateSet OCS(R.getNameLoc());
+        OverloadCandidateSet::iterator Best;
+        for (TypoCorrection::decl_iterator CD = Corrected.begin(),
+                                        CDEnd = Corrected.end();
+             CD != CDEnd; ++CD) {
+          assert(isa<FunctionDecl>(*CD) &&
+                 "Encountered a non-function as a function overload candidate");
+          AddOverloadCandidate(cast<FunctionDecl>(*CD),
+                               DeclAccessPair::make(*CD, AS_none),
+                               Args, NumArgs, OCS);
+        }

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).

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.

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".

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110803/4afc6279/attachment.html>