[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] Fix source range of destructor name
From: Olivier Goffart <ogoffart () kde ! org>
Date: 2014-01-16 12:13:43
Message-ID: 1904403.dPyzmHUlEb () finn
[Download RAW message or body]
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 <ogoffart@kde.org> wrote:
> > On Wednesday 15 January 2014 13:50:49 Richard Smith wrote:
> > > On Wed, Jan 15, 2014 at 1:25 PM, Olivier Goffart <ogoffart@kde.org>
> >
> > 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.)
["0001-Fix-printing-of-CXX-DeclarationName.patch" (0001-Fix-printing-of-CXX-DeclarationName.patch)]
From c8025b05ce508a118bdc6f37f547b1f465ffd115 Mon Sep 17 00:00:00 2001
From: Olivier Goffart <ogoffart@woboq.com>
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
["0002-Fix-source-range-of-the-destructor-name.patch" (0002-Fix-source-range-of-the-destructor-name.patch)]
From 7d626729e0fb27e5eb4a847ebcb6e48bf0dce726 Mon Sep 17 00:00:00 2001
From: Olivier Goffart <ogoffart@woboq.com>
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
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic