Hi, Thanks for the feed back. I've now split the patch into two. The first one fix the printing of the tag name, when printing DeclarationName. There was one more way to print declaration name, and i think they should follow the same pattern. And the second patch fix the issue that the destructor did not have an attached type location. With a test this time.
-- Olivier On Thursday 16 January 2014 00:00:49 Richard Smith wrote: > On Wed Jan 15 2014 at 2:29:52 PM, Olivier Goffart <[email protected]> wrote: > > On Wednesday 15 January 2014 13:50:49 Richard Smith wrote: > > > On Wed, Jan 15, 2014 at 1:25 PM, Olivier Goffart <[email protected]> > > > > wrote: > > > > Hi, > > > > > > > > This fixes http://llvm.org/bugs/show_bug.cgi?id=15125 > > > > The problem is that destructor->getNameInfo().getSourceRange() only > > > > contains > > > > the '~' and not the class name after it. > > > > > > > > This is why for example the destructor name is not highlighted in my > > > > code > > > > > > browser, only the '~' has a tooltip. > > > > > > > > http://code.woboq.org/userspace/llvm/tools/clang/ > > > > include/clang/AST/Declara > > > > > > tionName.h.html#_ZN5clang20Declarationisn'tNameTableD1Ev<http://code. > > > > woboq > > > > > > .org/userspace/llvm/tools/clang/include/clang/AST/ > > > > DeclarationName.h.html#_ > > > > > > ZN5clang20DeclarationNameTableD1Ev> > > > > > > + return CreateParsedType(MemberOfType, > > > + Context.getTrivialTypeSourceInfo(MemberOfType, > > > NameLoc)); > > > > > > return ParsedType::make(MemberOfType); > > > > > > Please delete the unreachable 'return' statement here. > > > > This is obviously a left over. > > > > > There are 3 other 'return ParsedType::make(...);' calls in this > > > function, > > > and they all seem to have the same issue; do these need changes too? > > > > Right, I'll change them too. > > > > > Also, you aren't including the location information from the > > > > CXXScopeSpec in > > > > > the produced TypeSourceInfo, so this still doesn't look exactly right > > > > (for > > > > > p->X::Y::~Y(), you probably won't be able to provide an appropriate > > > > tooltip > > > > > for the 'X', even with this fix, for instance). > > > > I fail to understand this. > > > > only ~Y here is the name of the destructor. > > If I am not mistaken, X::Y:: is the qualifier (as traversed with > > destructor- > > > > >getQualifierLoc()) and should not be part of the function name itself. > > Sorry, yes, you're absolutely right. > > > > The changes to DeclarationName.cpp and to DeclPrinterTest.cpp look > > > unrelated to this fix (right?), and the fix itself seems to be missing a > > > test. > > > > They are related because now that the destructor has a full type > > information, > > > > '~class Foo' is written instead of '~Foo' and other tests were failing > > > > without that change. > > OK, it's a shame that we need to do this, but this looks fine. It'd be a > little nicer to set LangOpts.CPlusPlus rather than setting > SuppressTagKeywords. > > (I wonder if we can remove the printing of the tag keyword from > TypePrinter::printTag -- it should be wrapped in an ElaboratedType if > that's necessary, and the tag keyword would get printed there.)
>From c8025b05ce508a118bdc6f37f547b1f465ffd115 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <[email protected]> Date: Thu, 16 Jan 2014 12:59:04 +0100 Subject: [PATCH 1/2] Fix printing of CXX DeclarationName Destructor ocould be printed '~class Foo' instead of just '~Foo', or 'operator struct Bar()' instead of 'operator Bar()' Use a C++ print policy to print the type names inside C++ constrcuts. --- lib/AST/DeclarationName.cpp | 16 ++++++++++++---- test/CXX/class.access/p4.cpp | 4 ++-- test/CXX/class.access/p6.cpp | 2 +- test/CXX/special/class.dtor/p10-0x.cpp | 2 +- test/Index/load-classes.cpp | 2 +- test/SemaCXX/undefined-internal.cpp | 2 +- unittests/AST/DeclPrinterTest.cpp | 2 +- 7 files changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/AST/DeclarationName.cpp b/lib/AST/DeclarationName.cpp index e064e23..3a8fa20 100644 --- a/lib/AST/DeclarationName.cpp +++ b/lib/AST/DeclarationName.cpp @@ -149,7 +149,9 @@ raw_ostream &operator<<(raw_ostream &OS, DeclarationName N) { QualType ClassType = N.getCXXNameType(); if (const RecordType *ClassRec = ClassType->getAs<RecordType>()) return OS << *ClassRec->getDecl(); - return OS << ClassType.getAsString(); + LangOptions LO; + LO.CPlusPlus = true; + return OS << ClassType.getAsString(PrintingPolicy(LO)); } case DeclarationName::CXXDestructorName: { @@ -157,7 +159,9 @@ raw_ostream &operator<<(raw_ostream &OS, DeclarationName N) { QualType Type = N.getCXXNameType(); if (const RecordType *Rec = Type->getAs<RecordType>()) return OS << *Rec->getDecl(); - return OS << Type.getAsString(); + LangOptions LO; + LO.CPlusPlus = true; + return OS << Type.getAsString(PrintingPolicy(LO)); } case DeclarationName::CXXOperatorName: { @@ -184,7 +188,9 @@ raw_ostream &operator<<(raw_ostream &OS, DeclarationName N) { QualType Type = N.getCXXNameType(); if (const RecordType *Rec = Type->getAs<RecordType>()) return OS << *Rec->getDecl(); - return OS << Type.getAsString(); + LangOptions LO; + LO.CPlusPlus = true; + return OS << Type.getAsString(PrintingPolicy(LO)); } case DeclarationName::CXXUsingDirective: return OS << "<using-directive>"; @@ -537,7 +543,9 @@ void DeclarationNameInfo::printName(raw_ostream &OS) const { OS << '~'; else if (Name.getNameKind() == DeclarationName::CXXConversionFunctionName) OS << "operator "; - OS << TInfo->getType().getAsString(); + LangOptions LO; + LO.CPlusPlus = true; + OS << TInfo->getType().getAsString(PrintingPolicy(LO)); } else OS << Name; return; diff --git a/test/CXX/class.access/p4.cpp b/test/CXX/class.access/p4.cpp index 0564a52..435b099 100644 --- a/test/CXX/class.access/p4.cpp +++ b/test/CXX/class.access/p4.cpp @@ -79,8 +79,8 @@ namespace test1 { -ca; // These are all surrogate calls ca(pub); - ca(prot); // expected-error {{'operator void (*)(class Protected &)' is a protected member}} - ca(priv); // expected-error {{'operator void (*)(class Private &)' is a private member}} + ca(prot); // expected-error {{'operator void (*)(Protected &)' is a protected member}} + ca(priv); // expected-error {{'operator void (*)(Private &)' is a private member}} } } diff --git a/test/CXX/class.access/p6.cpp b/test/CXX/class.access/p6.cpp index 5007263..f9b95f0 100644 --- a/test/CXX/class.access/p6.cpp +++ b/test/CXX/class.access/p6.cpp @@ -177,7 +177,7 @@ namespace test8 { }; void test(A &a) { - if (a) return; // expected-error-re {{'operator void *(class test8::A::*)(void){{( __attribute__\(\(thiscall\)\))?}} const' is a private member of 'test8::A'}} + if (a) return; // expected-error-re {{'operator void *(test8::A::*)(){{( __attribute__\(\(thiscall\)\))?}} const' is a private member of 'test8::A'}} } } diff --git a/test/CXX/special/class.dtor/p10-0x.cpp b/test/CXX/special/class.dtor/p10-0x.cpp index e10afb5..029cbd6 100644 --- a/test/CXX/special/class.dtor/p10-0x.cpp +++ b/test/CXX/special/class.dtor/p10-0x.cpp @@ -7,7 +7,7 @@ template<typename T> void b(const T *x, const A *y) { x->~decltype(T())(); x->~decltype(*x)(); // expected-error{{the type of object expression ('const int') does not match the type being destroyed ('decltype(*x)' (aka 'const int &')) in pseudo-destructor expression}} \ - expected-error{{no member named '~const struct A &' in 'A'}} + expected-error{{no member named '~const A &' in 'A'}} x->~decltype(int())(); // expected-error{{no member named '~int' in 'A'}} y->~decltype(*y)(); // expected-error{{destructor type 'decltype(*y)' (aka 'const A &') in object destruction expression does not match the type 'const A' of the object being destroyed}} diff --git a/test/Index/load-classes.cpp b/test/Index/load-classes.cpp index db7b48f..9790d9e 100644 --- a/test/Index/load-classes.cpp +++ b/test/Index/load-classes.cpp @@ -23,7 +23,7 @@ X::X(int value) { // CHECK: load-classes.cpp:5:11: TypeRef=struct X:3:8 Extent=[5:11 - 5:12] // CHECK: load-classes.cpp:7:3: CXXDestructor=~X:7:3 Extent=[7:3 - 7:7] [access=protected] // FIXME: missing TypeRef in the destructor name -// CHECK: load-classes.cpp:9:3: CXXConversion=operator struct X *:9:3 Extent=[9:3 - 9:16] [access=private] +// CHECK: load-classes.cpp:9:3: CXXConversion=operator X *:9:3 Extent=[9:3 - 9:16] [access=private] // CHECK: load-classes.cpp:9:12: TypeRef=struct X:3:8 Extent=[9:12 - 9:13] // CHECK: load-classes.cpp:12:4: CXXConstructor=X:12:4 (Definition) Extent=[12:1 - 13:2] [access=public] // CHECK: load-classes.cpp:12:1: TypeRef=struct X:3:8 Extent=[12:1 - 12:2] diff --git a/test/SemaCXX/undefined-internal.cpp b/test/SemaCXX/undefined-internal.cpp index 1b76a86..c653499 100644 --- a/test/SemaCXX/undefined-internal.cpp +++ b/test/SemaCXX/undefined-internal.cpp @@ -285,7 +285,7 @@ namespace test12 { virtual operator T3&() = 0; operator T4(); // expected-warning {{function 'test12::<anonymous namespace>::Cls::operator T4' has internal linkage but is not defined}} operator T5(); // expected-warning {{function 'test12::<anonymous namespace>::Cls::operator T5' has internal linkage but is not defined}} - operator T6&(); // expected-warning {{function 'test12::<anonymous namespace>::Cls::operator class test12::T6 &' has internal linkage but is not defined}} + operator T6&(); // expected-warning {{function 'test12::<anonymous namespace>::Cls::operator test12::T6 &' has internal linkage but is not defined}} }; struct Cls2 { diff --git a/unittests/AST/DeclPrinterTest.cpp b/unittests/AST/DeclPrinterTest.cpp index 44fa742..2e335e3 100644 --- a/unittests/AST/DeclPrinterTest.cpp +++ b/unittests/AST/DeclPrinterTest.cpp @@ -558,7 +558,7 @@ TEST(DeclPrinter, TestCXXConversionDecl3) { " operator Z();" "};", methodDecl(ofClass(hasName("A"))).bind("id"), - "Z operator struct Z()")); + "Z operator Z()")); // WRONG; Should be: "operator Z();" } -- 1.8.5.2
>From 7d626729e0fb27e5eb4a847ebcb6e48bf0dce726 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <[email protected]> Date: Wed, 15 Jan 2014 19:45:12 +0100 Subject: [PATCH 2/2] Fix source range of the destructor name. The problem is that the destructor's DeclarationNameInfo do not have a TypeSourceInfo because Sema::GetNameForDeclarator requires the ParsedType to be a LocInfoType. http://llvm.org/bugs/show_bug.cgi?id=15125 --- lib/Sema/SemaExprCXX.cpp | 15 +++++++++++---- test/SemaCXX/sourceranges.cpp | 9 +++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp index a0c123f..db4949c 100644 --- a/lib/Sema/SemaExprCXX.cpp +++ b/lib/Sema/SemaExprCXX.cpp @@ -209,7 +209,8 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc, Context.hasSameUnqualifiedType(T, SearchType)) { // We found our type! - return ParsedType::make(T); + return CreateParsedType(T, + Context.getTrivialTypeSourceInfo(T, NameLoc)); } if (!SearchType.isNull()) @@ -245,7 +246,9 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc, = dyn_cast<ClassTemplateSpecializationDecl>(Record->getDecl())) { if (Spec->getSpecializedTemplate()->getCanonicalDecl() == Template->getCanonicalDecl()) - return ParsedType::make(MemberOfType); + return CreateParsedType( + MemberOfType, + Context.getTrivialTypeSourceInfo(MemberOfType, NameLoc)); } continue; @@ -264,7 +267,9 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc, // specialized. if (TemplateDecl *SpecTemplate = SpecName.getAsTemplateDecl()) { if (SpecTemplate->getCanonicalDecl() == Template->getCanonicalDecl()) - return ParsedType::make(MemberOfType); + return CreateParsedType( + MemberOfType, + Context.getTrivialTypeSourceInfo(MemberOfType, NameLoc)); continue; } @@ -275,7 +280,9 @@ ParsedType Sema::getDestructorName(SourceLocation TildeLoc, = SpecName.getAsDependentTemplateName()) { if (DepTemplate->isIdentifier() && DepTemplate->getIdentifier() == Template->getIdentifier()) - return ParsedType::make(MemberOfType); + return CreateParsedType( + MemberOfType, + Context.getTrivialTypeSourceInfo(MemberOfType, NameLoc)); continue; } diff --git a/test/SemaCXX/sourceranges.cpp b/test/SemaCXX/sourceranges.cpp index 1f25d5b..900db89 100644 --- a/test/SemaCXX/sourceranges.cpp +++ b/test/SemaCXX/sourceranges.cpp @@ -28,3 +28,12 @@ foo::A getName() { // CHECK: CXXConstructExpr {{0x[0-9a-fA-F]+}} <col:10, col:17> 'foo::A' return foo::A(); } + +void destruct(foo::A *a1, foo::A *a2, P<int> *p1) { + // CHECK: MemberExpr {{0x[0-9a-fA-F]+}} <col:3, col:8> '<bound member function type>' ->~A + a1->~A(); + // CHECK: MemberExpr {{0x[0-9a-fA-F]+}} <col:3, col:16> '<bound member function type>' ->~A + a2->foo::A::~A(); + // CHECK: MemberExpr {{0x[0-9a-fA-F]+}} <col:3, col:13> '<bound member function type>' ->~P + p1->~P<int>(); +} \ No newline at end of file -- 1.8.5.2
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
