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 <dgre...@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 <chandl...@google.com>wrote:
>
>> On Thu, Jul 28, 2011 at 2:25 PM, Kaelyn Uhrain <ri...@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
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index cbb096d..08514cd 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -2235,7 +2235,8 @@ public:
const TemplateArgumentListInfo *&TemplateArgs);
bool DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
- CorrectTypoContext CTC = CTC_Unknown);
+ CorrectTypoContext CTC = CTC_Unknown,
+ Expr **Args = 0, unsigned NumArgs = 0);
ExprResult LookupInObjCMethod(LookupResult &R, Scope *S, IdentifierInfo *II,
bool AllowBuiltinCreation=false);
diff --git a/include/clang/Sema/TypoCorrection.h b/include/clang/Sema/TypoCorrection.h
index 9965953..480a71a 100644
--- a/include/clang/Sema/TypoCorrection.h
+++ b/include/clang/Sema/TypoCorrection.h
@@ -16,6 +16,7 @@
#define LLVM_CLANG_SEMA_TYPOCORRECTION_H
#include "clang/AST/DeclCXX.h"
+#include "llvm/ADT/SmallVector.h"
namespace clang {
@@ -23,29 +24,31 @@ namespace clang {
class TypoCorrection {
public:
TypoCorrection(const DeclarationName &Name, NamedDecl *NameDecl,
- NestedNameSpecifier *NNS=NULL, unsigned distance=0)
+ NestedNameSpecifier *NNS=0, unsigned distance=0)
: CorrectionName(Name),
CorrectionNameSpec(NNS),
- CorrectionDecl(NameDecl),
- EditDistance(distance) {}
+ EditDistance(distance) {
+ if (NameDecl)
+ CorrectionDecls.push_back(NameDecl);
+ }
- TypoCorrection(NamedDecl *Name, NestedNameSpecifier *NNS=NULL,
+ TypoCorrection(NamedDecl *Name, NestedNameSpecifier *NNS=0,
unsigned distance=0)
: CorrectionName(Name->getDeclName()),
CorrectionNameSpec(NNS),
- CorrectionDecl(Name),
- EditDistance(distance) {}
+ EditDistance(distance) {
+ if (Name)
+ CorrectionDecls.push_back(Name);
+ }
- TypoCorrection(DeclarationName Name, NestedNameSpecifier *NNS=NULL,
+ TypoCorrection(DeclarationName Name, NestedNameSpecifier *NNS=0,
unsigned distance=0)
: CorrectionName(Name),
CorrectionNameSpec(NNS),
- CorrectionDecl(NULL),
- EditDistance(distance) {}
+ EditDistance(distance) {}
TypoCorrection()
- : CorrectionName(), CorrectionNameSpec(NULL), CorrectionDecl(NULL),
- EditDistance(0) {}
+ : CorrectionNameSpec(0), EditDistance(0) {}
/// \brief Gets the DeclarationName of the typo correction
DeclarationName getCorrection() const { return CorrectionName; }
@@ -66,37 +69,67 @@ public:
/// \brief Gets the pointer to the declaration of the typo correction
NamedDecl* getCorrectionDecl() const {
- return isKeyword() ? NULL : CorrectionDecl;
+ return hasCorrectionDecl() ? *(CorrectionDecls.begin()) : 0;
}
template <class DeclClass>
DeclClass *getCorrectionDeclAs() const {
return dyn_cast_or_null<DeclClass>(getCorrectionDecl());
}
+ /// \brief Clears the list of NamedDecls before adding the new one.
void setCorrectionDecl(NamedDecl *CDecl) {
- CorrectionDecl = CDecl;
- if (!CorrectionName)
- CorrectionName = CDecl->getDeclName();
+ CorrectionDecls.clear();
+ addCorrectionDecl(CDecl);
}
+ /// \brief Add the given NamedDecl to the list of NamedDecls that are the
+ /// declarations associated with the DeclarationName of this TypoCorrection
+ void addCorrectionDecl(NamedDecl *CDecl);
+
std::string getAsString(const LangOptions &LO) const;
std::string getQuoted(const LangOptions &LO) const {
return "'" + getAsString(LO) + "'";
}
+ /// \brief Returns whether this TypoCorrection has a non-empty DeclarationName
operator bool() const { return bool(CorrectionName); }
- static inline NamedDecl *KeywordDecl() { return (NamedDecl*)-1; }
- bool isKeyword() const { return CorrectionDecl == KeywordDecl(); }
+ /// \brief Mark this TypoCorrection as being a keyword.
+ /// Since addCorrectionDeclsand setCorrectionDecl don't allow NULL to be
+ /// added to the list of the correction's NamedDecl pointers, NULL is added
+ /// as the only element in the list to mark this TypoCorrection as a keyword.
+ void makeKeyword() {
+ CorrectionDecls.clear();
+ CorrectionDecls.push_back(0);
+ }
+
+ // Check if this TypoCorrection is a keyword by checking if the first
+ // item in CorrectionDecls is NULL.
+ bool isKeyword() const {
+ return !CorrectionDecls.empty() &&
+ CorrectionDecls.front() == 0;
+ }
// Returns true if the correction either is a keyword or has a known decl.
- bool isResolved() const { return CorrectionDecl != NULL; }
+ bool isResolved() const { return !CorrectionDecls.empty(); }
+
+ bool isOverloaded() const {
+ return CorrectionDecls.size() > 1;
+ }
+
+ typedef llvm::SmallVector<NamedDecl*, 1>::iterator decl_iterator;
+ decl_iterator begin() { return CorrectionDecls.begin(); }
+ decl_iterator end() { return CorrectionDecls.end(); }
private:
+ bool hasCorrectionDecl() const {
+ return (!isKeyword() && !CorrectionDecls.empty());
+ }
+
// Results.
DeclarationName CorrectionName;
NestedNameSpecifier *CorrectionNameSpec;
- NamedDecl *CorrectionDecl;
+ llvm::SmallVector<NamedDecl*, 1> CorrectionDecls;
unsigned EditDistance;
};
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index bfaf546..d3462f2 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -1333,7 +1333,8 @@ void Sema::DecomposeUnqualifiedId(const UnqualifiedId &Id,
///
/// \return false if new lookup candidates were found
bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
- CorrectTypoContext CTC) {
+ CorrectTypoContext CTC, Expr **Args,
+ unsigned NumArgs) {
DeclarationName Name = R.getLookupName();
unsigned diagnostic = diag::err_undeclared_var_use;
@@ -1419,6 +1420,27 @@ bool Sema::DiagnoseEmptyLookup(Scope *S, CXXScopeSpec &SS, LookupResult &R,
R.setLookupName(Corrected.getCorrection());
if (NamedDecl *ND = Corrected.getCorrectionDecl()) {
+ if (Corrected.isOverloaded()) {
+ OverloadCandidateSet OCS(R.getNameLoc());
+ OverloadCandidateSet::iterator Best;
+ for (TypoCorrection::decl_iterator CD = Corrected.begin(),
+ CDEnd = Corrected.end();
+ CD != CDEnd; ++CD) {
+ if (FunctionDecl *FD = dyn_cast<FunctionDecl>(*CD))
+ AddOverloadCandidate(FD, DeclAccessPair::make(*CD, AS_none),
+ Args, NumArgs, OCS);
+ // TODO: Handle FunctionTemplateDecl and other Decl types that
+ // support overloading and could be corrected by CorrectTypo.
+ }
+ switch (OCS.BestViableFunction(*this, R.getNameLoc(), Best)) {
+ case OR_Success:
+ ND = Best->Function;
+ break;
+ default:
+ // Don't try to recover; it won't work.
+ return true;
+ }
+ }
R.addDecl(ND);
if (isa<ValueDecl>(ND) || isa<FunctionTemplateDecl>(ND)) {
if (SS.isEmpty())
diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index c5ba95a..240eb5f 100644
--- a/lib/Sema/SemaLookup.cpp
+++ b/lib/Sema/SemaLookup.cpp
@@ -3020,7 +3020,7 @@ public:
void FoundName(StringRef Name);
void addKeywordResult(StringRef Keyword);
void addName(StringRef Name, NamedDecl *ND, unsigned Distance,
- NestedNameSpecifier *NNS=NULL);
+ NestedNameSpecifier *NNS=NULL, bool isKeyword=false);
void addCorrection(TypoCorrection Correction);
typedef TypoResultsMap::iterator result_iterator;
@@ -3099,15 +3099,17 @@ void TypoCorrectionConsumer::addKeywordResult(StringRef Keyword) {
return;
}
- addName(Keyword, TypoCorrection::KeywordDecl(), ED);
+ addName(Keyword, NULL, ED, NULL, true);
}
void TypoCorrectionConsumer::addName(StringRef Name,
NamedDecl *ND,
unsigned Distance,
- NestedNameSpecifier *NNS) {
- addCorrection(TypoCorrection(&SemaRef.Context.Idents.get(Name),
- ND, NNS, Distance));
+ NestedNameSpecifier *NNS,
+ bool isKeyword) {
+ TypoCorrection TC(&SemaRef.Context.Idents.get(Name), ND, NNS, Distance);
+ if (isKeyword) TC.makeKeyword();
+ addCorrection(TC);
}
void TypoCorrectionConsumer::addCorrection(TypoCorrection Correction) {
@@ -3677,12 +3679,19 @@ TypoCorrection Sema::CorrectTypo(const DeclarationNameInfo &TypoName,
// We don't deal with ambiguities.
return TypoCorrection();
+ case LookupResult::FoundOverloaded: {
+ // Store all of the Decls for overloaded symbols
+ for (LookupResult::iterator TRD = TmpRes.begin(),
+ TRDEnd = TmpRes.end();
+ TRD != TRDEnd; ++TRD)
+ I->second.addCorrectionDecl(*TRD);
+ ++I;
+ break;
+ }
+
case LookupResult::Found:
- case LookupResult::FoundOverloaded:
case LookupResult::FoundUnresolvedValue:
I->second.setCorrectionDecl(TmpRes.getAsSingle<NamedDecl>());
- // FIXME: This sets the CorrectionDecl to NULL for overloaded functions.
- // It would be nice to find the right one with overload resolution.
++I;
break;
}
@@ -3718,11 +3727,20 @@ TypoCorrection Sema::CorrectTypo(const DeclarationNameInfo &TypoName,
switch (TmpRes.getResultKind()) {
case LookupResult::Found:
- case LookupResult::FoundOverloaded:
case LookupResult::FoundUnresolvedValue:
Consumer.addName((*QRI)->getName(), TmpRes.getAsSingle<NamedDecl>(),
QualifiedED, NI->NameSpecifier);
break;
+ case LookupResult::FoundOverloaded: {
+ TypoCorrection corr(&Context.Idents.get((*QRI)->getName()), NULL,
+ NI->NameSpecifier, QualifiedED);
+ for (LookupResult::iterator TRD = TmpRes.begin(),
+ TRDEnd = TmpRes.end();
+ TRD != TRDEnd; ++TRD)
+ corr.addCorrectionDecl(*TRD);
+ Consumer.addCorrection(corr);
+ break;
+ }
case LookupResult::NotFound:
case LookupResult::NotFoundInCurrentInstantiation:
case LookupResult::Ambiguous:
@@ -3802,6 +3820,18 @@ TypoCorrection Sema::CorrectTypo(const DeclarationNameInfo &TypoName,
return TypoCorrection();
}
+void TypoCorrection::addCorrectionDecl(NamedDecl *CDecl) {
+ if (!CDecl) return;
+
+ if (isKeyword())
+ CorrectionDecls.clear();
+
+ CorrectionDecls.push_back(CDecl);
+
+ if (!CorrectionName)
+ CorrectionName = CDecl->getDeclName();
+}
+
std::string TypoCorrection::getAsString(const LangOptions &LO) const {
if (CorrectionNameSpec) {
std::string tmpBuffer;
diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp
index 79aa305..712720b 100644
--- a/lib/Sema/SemaOverload.cpp
+++ b/lib/Sema/SemaOverload.cpp
@@ -8220,7 +8220,8 @@ BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
if (!DiagnoseTwoPhaseLookup(SemaRef, Fn->getExprLoc(), SS, R,
ExplicitTemplateArgs, Args, NumArgs) &&
(!EmptyLookup ||
- SemaRef.DiagnoseEmptyLookup(S, SS, R, Sema::CTC_Expression)))
+ SemaRef.DiagnoseEmptyLookup(S, SS, R, Sema::CTC_Expression,
+ Args, NumArgs)))
return ExprError();
assert(!R.empty() && "lookup results empty despite recovery");
diff --git a/test/FixIt/typo-crash.cpp b/test/FixIt/typo-crash.cpp
index b156e1b..8f0c989 100644
--- a/test/FixIt/typo-crash.cpp
+++ b/test/FixIt/typo-crash.cpp
@@ -3,9 +3,10 @@
// FIXME: The diagnostics and recovery here are very, very poor.
// PR10355
-template<typename T> void template_id1() {
- template_id2<> t; // expected-error 2{{use of undeclared identifier 'template_id2'; did you mean 'template_id1'?}} \
- // expected-error{{expected expression}} \
- // expected-error{{use of undeclared identifier 't'}}
+template<typename T> void template_id1() { // expected-note {{'template_id1' declared here}} \
+ // expected-note {{candidate function}}
+ template_id2<> t; // expected-error {{no template named 'template_id2'; did you mean 'template_id1'?}} \
+ // expected-error {{expected ';' after expression}} \
+ // expected-error {{cannot resolve overloaded function 'template_id1' from context}} \
+ // expected-error {{use of undeclared identifier 't'}}
}
-
diff --git a/test/SemaCXX/function-overload-typo-crash.cpp b/test/SemaCXX/function-overload-typo-crash.cpp
index 0fea312..580f27a 100644
--- a/test/SemaCXX/function-overload-typo-crash.cpp
+++ b/test/SemaCXX/function-overload-typo-crash.cpp
@@ -1,10 +1,10 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// PR10283
-void min();
+void min(); //expected-note {{'min' declared here}}
void min(int);
-template <typename T> void max(T);
+template <typename T> void max(T); //expected-note {{'max' declared here}}
void f() {
fin(); //expected-error {{use of undeclared identifier 'fin'; did you mean 'min'}}
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
