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

[Kaelyn Uhrain]

Doug,

Thanks for the comments! I've attached an updated patch that includes a few
small changes based on your feedback.

On Wed, Aug 3, 2011 at 10:29 AM, Douglas Gregor <dgregor at apple.com> wrote:

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

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.


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

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


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

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.

Thanks,
Kaelyn
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110803/0069bda5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: typo-correction-overload4.diff
Type: text/x-diff
Size: 13478 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110803/0069bda5/attachment.diff>