And here is the latest version of this patch, with the new test cases added.
On Fri, Oct 10, 2014 at 1:30 PM, Kaelyn Takata <[email protected]> wrote:
>
>
> On Tue, Oct 7, 2014 at 4:24 PM, David Blaikie <[email protected]> wrote:
>
>>
>>
>> On Tue, Oct 7, 2014 at 3:30 PM, Kaelyn Takata <[email protected]> wrote:
>>
>>> On Fri, Oct 3, 2014 at 12:13 PM, David Blaikie <[email protected]>
>>> wrote:
>>>
>>>> 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 (const
>>>> TypoCorrection &TC)> instead. The callers would then just pass in lambdas
>>>> and std::function would provide the type erasure, dynamic destruction, etc.
>>>>
>>>> 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)
>>>>
>>>> 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).
>>>
>>
>> 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:
>>
>> Remove TypoDiagnosticGenerator
>> Use std::function<void (const TypoCorrection&)> where you had
>> unique_ptr<TypoDiagnosticGenerator>
>> Existing TypoDiagnosticGenerator subclasses can be modified to not
>> inherit from anything, and remove the "override" keyword from operator().
>> Any existing TypoDiagnosticGenerator subclasses that are simple enough,
>> could be inlined into lambdas (capturing whatever state they need), if
>> desired.
>>
>> 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.
>>
>> (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 */ }))
>>
>> Anyway, up to you - just a minor change, really. Bit more C++-y.
>>
>
> 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).
>
>>
>>
>>> 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
>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140818/113266.html
>>> is
>>> longer than MemberExprTypoRecovery and has state that goes beyond simply
>>> capturing variables).
>>>
>>>> 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.
>>>>
>>>> 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):
>>>
>>> // Extract the NamedDecl from the transformed TypoExpr and add
>>> it to the
>>> // TypoCorrection, replacing the existing decls. This ensures
>>> the right
>>> // NamedDecl is used in diagnostics e.g. in the case where
>>> overload
>>> // resolution was used to select one from several possible decls
>>> that
>>> // had been stored in the TypoCorrection.
>>>
>>
>> Ah, I guess that's the kicker - overload resolution, etc. Thanks muchly.
>>
>
> You're welcome. :)
>
>>
>>
>>> if (auto *ND = getDeclFromExpr(
>>> Replacement.isInvalid() ? nullptr : Replacement.get()))
>>> TC.setCorrectionDecl(ND);
>>>
>>>
>>>> On Thu, Sep 25, 2014 at 2:00 PM, Kaelyn Takata <[email protected]>
>>>> wrote:
>>>>
>>>>> ping
>>>>>
>>>>> On Tue, Aug 26, 2014 at 11:06 AM, Kaelyn Takata <[email protected]>
>>>>> wrote:
>>>>>
>>>>>> +dblaikie
>>>>>>
>>>>>>
>>>>>> On Thu, Jul 31, 2014 at 1:20 PM, Kaelyn Takata <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> This includes adding the new TypoExpr-based lazy typo correction to
>>>>>>> LookupMemberExprInRecord as an alternative to the existing eager typo
>>>>>>> correction.
>>>>>>> ---
>>>>>>> lib/Sema/SemaExprCXX.cpp | 40 ++++++++++-
>>>>>>> lib/Sema/SemaExprMember.cpp | 116
>>>>>>> ++++++++++++++++++++++++++++---
>>>>>>> test/SemaCXX/arrow-operator.cpp | 5 +-
>>>>>>> test/SemaCXX/typo-correction-delayed.cpp | 32 +++++++++
>>>>>>> test/SemaCXX/typo-correction-pt2.cpp | 2 +-
>>>>>>> test/SemaCXX/typo-correction.cpp | 10 +--
>>>>>>> 6 files changed, 186 insertions(+), 19 deletions(-)
>>>>>>> create mode 100644 test/SemaCXX/typo-correction-delayed.cpp
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
index 1d9df2d..9e05e82 100644
--- a/lib/Sema/SemaExprCXX.cpp
+++ b/lib/Sema/SemaExprCXX.cpp
@@ -5921,6 +5921,7 @@ class TransformTypos : public TreeTransform<TransformTypos> {
llvm::SmallSetVector<TypoExpr *, 2> TypoExprs;
llvm::SmallDenseMap<TypoExpr *, ExprResult, 2> TransformCache;
+ llvm::SmallDenseMap<OverloadExpr *, Expr *, 4> OverloadResolution;
/// \brief Emit diagnostics for all of the TypoExprs encountered.
/// If the TypoExprs were successfully corrected, then the diagnostics should
@@ -5930,8 +5931,21 @@ class TransformTypos : public TreeTransform<TransformTypos> {
for (auto E : TypoExprs) {
TypoExpr *TE = cast<TypoExpr>(E);
auto &State = SemaRef.getTypoExprState(TE);
- if (State.DiagHandler)
- State.DiagHandler(State.Consumer->getCurrentCorrection());
+ if (State.DiagHandler) {
+ TypoCorrection TC = State.Consumer->getCurrentCorrection();
+ ExprResult Replacement = TransformCache[TE];
+
+ // Extract the NamedDecl from the transformed TypoExpr and add it to the
+ // TypoCorrection, replacing the existing decls. This ensures the right
+ // NamedDecl is used in diagnostics e.g. in the case where overload
+ // resolution was used to select one from several possible decls that
+ // had been stored in the TypoCorrection.
+ if (auto *ND = getDeclFromExpr(
+ Replacement.isInvalid() ? nullptr : Replacement.get()))
+ TC.setCorrectionDecl(ND);
+
+ State.DiagHandler(TC);
+ }
SemaRef.clearDelayedTypo(TE);
}
}
@@ -5956,9 +5970,38 @@ class TransformTypos : public TreeTransform<TransformTypos> {
return false;
}
+ NamedDecl *getDeclFromExpr(Expr *E) {
+ if (auto *OE = dyn_cast_or_null<OverloadExpr>(E))
+ E = OverloadResolution[OE];
+
+ if (!E)
+ return nullptr;
+ if (auto *DRE = dyn_cast<DeclRefExpr>(E))
+ return DRE->getDecl();
+ if (auto *ME = dyn_cast<MemberExpr>(E))
+ return ME->getMemberDecl();
+ // FIXME: Add any other expr types that could be be seen by the delayed typo
+ // correction TreeTransform for which the corresponding TypoCorrection could
+ // contain multple decls.
+ return nullptr;
+ }
+
public:
TransformTypos(Sema &SemaRef) : BaseTransform(SemaRef) {}
+ ExprResult RebuildCallExpr(Expr *Callee, SourceLocation LParenLoc,
+ MultiExprArg Args,
+ SourceLocation RParenLoc,
+ Expr *ExecConfig = nullptr) {
+ auto Result = BaseTransform::RebuildCallExpr(Callee, LParenLoc, Args,
+ RParenLoc, ExecConfig);
+ if (auto *OE = dyn_cast<OverloadExpr>(Callee)) {
+ if (!Result.isInvalid() && Result.get())
+ OverloadResolution[OE] = cast<CallExpr>(Result.get())->getCallee();
+ }
+ return Result;
+ }
+
ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E); }
ExprResult Transform(Expr *E) {
diff --git a/lib/Sema/SemaExprMember.cpp b/lib/Sema/SemaExprMember.cpp
index aca3df1..0d7f3c6 100644
--- a/lib/Sema/SemaExprMember.cpp
+++ b/lib/Sema/SemaExprMember.cpp
@@ -10,6 +10,7 @@
// This file implements semantic analysis member access expressions.
//
//===----------------------------------------------------------------------===//
+#include "clang/Sema/Overload.h"
#include "clang/Sema/SemaInternal.h"
#include "clang/AST/ASTLambda.h"
#include "clang/AST/DeclCXX.h"
@@ -569,13 +570,47 @@ class RecordMemberExprValidatorCCC : public CorrectionCandidateCallback {
const RecordDecl *const Record;
};
+class MemberTypoDiags {
+ Sema &SemaRef;
+ DeclContext *Ctx;
+ DeclarationName Typo;
+ SourceLocation TypoLoc;
+ SourceRange BaseRange;
+ SourceRange ScopeSpecLoc;
+ unsigned DiagnosticID;
+ unsigned NoSuggestDiagnosticID;
+
+public:
+ MemberTypoDiags(Sema &SemaRef, DeclContext *Ctx, DeclarationName Typo,
+ SourceLocation TypoLoc, SourceRange BaseRange,
+ CXXScopeSpec &SS, unsigned DiagnosticID,
+ unsigned NoSuggestDiagnosticID)
+ : SemaRef(SemaRef), Ctx(Ctx), Typo(Typo), TypoLoc(TypoLoc), BaseRange(BaseRange),
+ ScopeSpecLoc(SS.getRange()), DiagnosticID(DiagnosticID),
+ NoSuggestDiagnosticID(NoSuggestDiagnosticID) {}
+
+ void operator()(const TypoCorrection &TC) {
+ if (TC) {
+ assert(!TC.isKeyword() && "Got a keyword as a correction for a member!");
+ bool DroppedSpecifier =
+ TC.WillReplaceSpecifier() &&
+ Typo.getAsString() == TC.getAsString(SemaRef.getLangOpts());
+ SemaRef.diagnoseTypo(TC, SemaRef.PDiag(DiagnosticID) << Typo << Ctx
+ << DroppedSpecifier
+ << ScopeSpecLoc);
+ } else {
+ SemaRef.Diag(TypoLoc, NoSuggestDiagnosticID) << Typo << Ctx << BaseRange;
+ }
+ }
+};
+
}
-static bool
-LookupMemberExprInRecord(Sema &SemaRef, LookupResult &R,
- SourceRange BaseRange, const RecordType *RTy,
- SourceLocation OpLoc, CXXScopeSpec &SS,
- bool HasTemplateArgs) {
+static bool LookupMemberExprInRecord(
+ Sema &SemaRef, LookupResult &R, SourceRange BaseRange,
+ const RecordType *RTy, SourceLocation OpLoc, CXXScopeSpec &SS,
+ bool HasTemplateArgs, TypoExpr **TE = nullptr,
+ Sema::TypoRecoveryCallback TRC = nullptr) {
RecordDecl *RDecl = RTy->getDecl();
if (!SemaRef.isThisOutsideMemberFunctionBody(QualType(RTy, 0)) &&
SemaRef.RequireCompleteType(OpLoc, QualType(RTy, 0),
@@ -619,6 +654,20 @@ LookupMemberExprInRecord(Sema &SemaRef, LookupResult &R,
if (!R.empty())
return false;
+ if (TE && SemaRef.getLangOpts().CPlusPlus) {
+ // TODO: C cannot handle TypoExpr nodes because the C code paths do not know
+ // what to do with dependent types e.g. on the LHS of an assigment.
+ *TE = SemaRef.CorrectTypoDelayed(
+ R.getLookupNameInfo(), R.getLookupKind(), nullptr, &SS,
+ llvm::make_unique<RecordMemberExprValidatorCCC>(RTy),
+ MemberTypoDiags(SemaRef, DC, R.getLookupName(), R.getNameLoc(),
+ BaseRange, SS, diag::err_no_member_suggest,
+ diag::err_no_member),
+ TRC, Sema::CTK_ErrorRecovery, DC);
+ R.clear();
+ return false;
+ }
+
// We didn't find anything with the given name, so try to correct
// for typos.
DeclarationName Name = R.getLookupName();
@@ -1148,6 +1197,48 @@ Sema::PerformMemberExprBaseConversion(Expr *Base, bool IsArrow) {
return CheckPlaceholderExpr(Base);
}
+namespace {
+
+class MemberExprTypoRecovery {
+ Expr *BaseExpr;
+ CXXScopeSpec SS;
+ SourceLocation OpLoc;
+ bool IsArrow;
+
+public:
+ MemberExprTypoRecovery(Expr *BE, CXXScopeSpec &SS, SourceLocation OpLoc,
+ bool IsArrow)
+ : BaseExpr(BE), SS(SS),
+ OpLoc(OpLoc), IsArrow(IsArrow) {}
+
+ ExprResult operator()(Sema &SemaRef, TypoExpr *TE, TypoCorrection TC) {
+ if (TC.isKeyword())
+ return ExprError();
+
+ LookupResult R(SemaRef, TC.getCorrection(),
+ TC.getCorrectionRange().getBegin(),
+ SemaRef.getTypoExprState(TE)
+ .Consumer->getLookupResult()
+ .getLookupKind());
+ R.suppressDiagnostics();
+
+ QualType BaseType;
+ if (auto *DRE = dyn_cast<DeclRefExpr>(BaseExpr))
+ BaseType = DRE->getDecl()->getType();
+ else if (auto *CE = dyn_cast<CallExpr>(BaseExpr))
+ BaseType = CE->getCallReturnType();
+
+ for (NamedDecl *ND : TC)
+ R.addDecl(ND);
+ R.resolveKind();
+ return SemaRef.BuildMemberReferenceExpr(
+ BaseExpr, BaseExpr->getType(), OpLoc, IsArrow, SS, SourceLocation(),
+ nullptr, R, nullptr);
+ }
+};
+
+}
+
/// Look up the given member of the given non-type-dependent
/// expression. This can return in one of two ways:
/// * If it returns a sentinel null-but-valid result, the caller will
@@ -1210,13 +1301,18 @@ static ExprResult LookupMemberExpr(Sema &S, LookupResult &R,
// Handle field access to simple records.
if (const RecordType *RTy = BaseType->getAs<RecordType>()) {
- if (LookupMemberExprInRecord(S, R, BaseExpr.get()->getSourceRange(),
- RTy, OpLoc, SS, HasTemplateArgs))
+ TypoExpr *TE = nullptr;
+ if (LookupMemberExprInRecord(
+ S, R, BaseExpr.get()->getSourceRange(), RTy, OpLoc, SS,
+ HasTemplateArgs, &TE,
+ MemberExprTypoRecovery(BaseExpr.get(), SS, OpLoc, IsArrow)))
return ExprError();
// Returning valid-but-null is how we indicate to the caller that
- // the lookup result was filled in.
- return ExprResult((Expr *)nullptr);
+ // the lookup result was filled in. If typo correction was attempted and
+ // failed, the lookup result will have been cleared--that combined with the
+ // valid-but-null ExprResult will trigger the appropriate diagnostics.
+ return ExprResult(TE);
}
// Handle ivar access to Objective-C objects.
diff --git a/test/SemaCXX/arrow-operator.cpp b/test/SemaCXX/arrow-operator.cpp
index 173ff72..3e32a6b 100644
--- a/test/SemaCXX/arrow-operator.cpp
+++ b/test/SemaCXX/arrow-operator.cpp
@@ -52,14 +52,15 @@ class wrapped_ptr {
class Worker {
public:
- void DoSomething();
+ void DoSomething(); // expected-note {{'DoSomething' declared here}}
void Chuck();
};
void test() {
wrapped_ptr<Worker> worker(new Worker);
worker.DoSomething(); // expected-error {{no member named 'DoSomething' in 'arrow_suggest::wrapped_ptr<arrow_suggest::Worker>'; did you mean to use '->' instead of '.'?}}
- worker.DoSamething(); // expected-error {{no member named 'DoSamething' in 'arrow_suggest::wrapped_ptr<arrow_suggest::Worker>'}}
+ worker.DoSamething(); // expected-error {{no member named 'DoSamething' in 'arrow_suggest::wrapped_ptr<arrow_suggest::Worker>'; did you mean to use '->' instead of '.'?}} \
+ // expected-error {{no member named 'DoSamething' in 'arrow_suggest::Worker'; did you mean 'DoSomething'?}}
worker.Chuck(); // expected-error {{no member named 'Chuck' in 'arrow_suggest::wrapped_ptr<arrow_suggest::Worker>'; did you mean 'Check'?}}
}
diff --git a/test/SemaCXX/typo-correction-delayed.cpp b/test/SemaCXX/typo-correction-delayed.cpp
new file mode 100644
index 0000000..c79fe45
--- /dev/null
+++ b/test/SemaCXX/typo-correction-delayed.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-c++11-extensions %s
+
+struct A {};
+struct B {};
+struct D {
+ A fizbin; // expected-note 2 {{declared here}}
+ A foobar; // expected-note 2 {{declared here}}
+ B roxbin; // expected-note 2 {{declared here}}
+ B toobad; // expected-note 2 {{declared here}}
+ void BooHoo();
+ void FoxBox();
+};
+
+void something(A, B);
+void test() {
+ D obj;
+ something(obj.fixbin, // expected-error {{did you mean 'fizbin'?}}
+ obj.toobat); // expected-error {{did you mean 'toobad'?}}
+ something(obj.toobat, // expected-error {{did you mean 'foobar'?}}
+ obj.fixbin); // expected-error {{did you mean 'roxbin'?}}
+ something(obj.fixbin, // expected-error {{did you mean 'fizbin'?}}
+ obj.fixbin); // expected-error {{did you mean 'roxbin'?}}
+ something(obj.toobat, // expected-error {{did you mean 'foobar'?}}
+ obj.toobat); // expected-error {{did you mean 'toobad'?}}
+ // Both members could be corrected to methods, but that isn't valid.
+ something(obj.boohoo, // expected-error-re {{no member named 'boohoo' in 'D'{{$}}}}
+ obj.foxbox); // expected-error-re {{no member named 'foxbox' in 'D'{{$}}}}
+ // The first argument has a usable correction but the second doesn't.
+ something(obj.boobar, // expected-error-re {{no member named 'boobar' in 'D'{{$}}}}
+ obj.foxbox); // expected-error-re {{no member named 'foxbox' in 'D'{{$}}}}
+}
+
+// Ensure the delayed typo correction does the right thing when trying to
+// recover using a seemingly-valid correction for which a valid expression to
+// replace the TypoExpr cannot be created (but which does have a second
+// correction candidate that would be a valid and usable correction).
+class Foo {
+public:
+ template <> void testIt(); // expected-error {{no function template matches}}
+ void textIt(); // expected-note {{'textIt' declared here}}
+};
+void testMemberExpr(Foo *f) {
+ f->TestIt(); // expected-error {{no member named 'TestIt' in 'Foo'; did you mean 'textIt'?}}
+}
diff --git a/test/SemaCXX/typo-correction-pt2.cpp b/test/SemaCXX/typo-correction-pt2.cpp
index d300764..de12da7 100644
--- a/test/SemaCXX/typo-correction-pt2.cpp
+++ b/test/SemaCXX/typo-correction-pt2.cpp
@@ -28,7 +28,7 @@ struct A {
};
struct B : A {
using A::CreateFoo;
- void CreateFoo(int, int);
+ void CreateFoo(int, int); // expected-note {{'CreateFoo' declared here}}
};
void f(B &x) {
x.Createfoo(0,0); // expected-error {{no member named 'Createfoo' in 'PR13387::B'; did you mean 'CreateFoo'?}}
diff --git a/test/SemaCXX/typo-correction.cpp b/test/SemaCXX/typo-correction.cpp
index e8160b0..a4f5174 100644
--- a/test/SemaCXX/typo-correction.cpp
+++ b/test/SemaCXX/typo-correction.cpp
@@ -247,7 +247,7 @@ namespace b6956809_test1 {
struct S1 {
void method(A*); // no note here
- void method(B*);
+ void method(B*); // expected-note{{'method' declared here}}
};
void test1() {
@@ -258,15 +258,15 @@ namespace b6956809_test1 {
struct S2 {
S2();
- void method(A*) const; // expected-note{{candidate function not viable}}
+ void method(A*) const;
private:
- void method(B*); // expected-note{{candidate function not viable}}
+ void method(B*);
};
void test2() {
B b;
const S2 s;
- s.methodd(&b); // expected-error{{no member named 'methodd' in 'b6956809_test1::S2'; did you mean 'method'}} expected-error{{no matching member function for call to 'method'}}
+ s.methodd(&b); // expected-error-re{{no member named 'methodd' in 'b6956809_test1::S2'{{$}}}}
}
}
@@ -274,7 +274,7 @@ namespace b6956809_test2 {
template<typename T> struct Err { typename T::error n; }; // expected-error{{type 'void *' cannot be used prior to '::' because it has no members}}
struct S {
template<typename T> typename Err<T>::type method(T); // expected-note{{in instantiation of template class 'b6956809_test2::Err<void *>' requested here}}
- template<typename T> int method(T *);
+ template<typename T> int method(T *); // expected-note{{'method' declared here}}
};
void test() {
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
