[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: Richard Smith <metafoo () gmail ! com>
Date: 2014-01-15 21:55:05
Message-ID: 6438654087761128549 () gmail297201516
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Wed Jan 15 2014 at 1:51:41 PM, Justin Bogner <mail@justinbogner.com>
wrote:
> Olivier Goffart <ogoffart@kde.org> writes:
> > From d131af55d2dc2cb6b2f7242697eb44d2e5d042da Mon Sep 17 00:00:00 2001
> > From: Olivier Goffart <ogoffart@woboq.com>
> > Date: Wed, 15 Jan 2014 19:45:12 +0100
> > Subject: [PATCH] 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.
> >
> > Setting a proper TypeSourceInfo to the destructor changes the way it
> > it printed (from '~Foo' to '~struct Foo'. Hence the change in
> > DeclarationName.cpp which also fix a bug when printing operator names.
> >
> > http://llvm.org/bugs/show_bug.cgi?id=15125
>
> Please add a test with this. Some minor comments below.
>
> > ---
> > lib/AST/DeclarationName.cpp | 5 ++++-
> > lib/Sema/SemaExprCXX.cpp | 5 ++++-
> > unittests/AST/DeclPrinterTest.cpp | 2 +-
> > 3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/AST/DeclarationName.cpp b/lib/AST/DeclarationName.cpp
> > index e064e23..3811bbf 100644
> > --- a/lib/AST/DeclarationName.cpp
> > +++ b/lib/AST/DeclarationName.cpp
> > @@ -537,7 +537,10 @@ void DeclarationNameInfo::printName(raw_ostream
> &OS) const {
> > OS << '~';
> > else if (Name.getNameKind() == DeclarationName::
> CXXConversionFunctionName)
> > OS << "operator ";
> > - OS << TInfo->getType().getAsString();
> > + LangOptions LO;
> > + PrintingPolicy SubPolicy(LO);
> > + SubPolicy.SuppressTagKeyword = true;
> > + OS << TInfo->getType().getAsString(SubPolicy);
> > } else
> > OS << Name;
> > return;
> > diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp
> > index a0c123f..7e4f5f3 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));
>
> This reads better as
>
> return CreateParsedType(T,
> Context.getTrivialTypeSourceInfo(T,
> NameLoc));
>
> > }
> >
> > if (!SearchType.isNull())
> > @@ -245,6 +246,8 @@ ParsedType Sema::getDestructorName(SourceLocation
> TildeLoc,
> > = dyn_cast<ClassTemplateSpecializationDecl>(Record->getDecl()))
> {
> > if (Spec->getSpecializedTemplate()->getCanonicalDecl() ==
> > Template->getCanonicalDecl())
> > + return CreateParsedType(MemberOfType,
> > + Context.getTrivialTypeSourceInfo(MemberOfType,
> NameLoc));
>
> This is a strange way to indent this. According to clang-format, this
> should be:
>
> return CreateParsedType(
> MemberOfType,
> Context.getTrivialTypeSourceInfo(MemberOfType, NameLoc));
>
> > return ParsedType::make(MemberOfType);
> > }
> >
> > 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();"
> > }
>
> Looks like the comment here needs updating.
>
The comment is still appropriate: we shouldn't be printing a return type.
[Attachment #5 (text/html)]
<div>On Wed Jan 15 2014 at 1:51:41 PM, Justin Bogner <<a \
href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> \
wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex"> Olivier Goffart <<a href="mailto:ogoffart@kde.org" \
target="_blank">ogoffart@kde.org</a>> writes:<br> > From \
d131af55d2dc2cb6b2f7242697eb44<u></u>d2e5d042da Mon Sep 17 00:00:00 2001<br> > \
From: Olivier Goffart <<a href="mailto:ogoffart@woboq.com" \
target="_blank">ogoffart@woboq.com</a>><br> > Date: Wed, 15 Jan 2014 19:45:12 \
+0100<br> > Subject: [PATCH] Fix source range of the destructor name.<br>
><br>
> The problem is that the destructor's DeclarationNameInfo do not have<br>
> a TypeSourceInfo because Sema::GetNameForDeclarator requires the<br>
> ParsedType to be a LocInfoType.<br>
><br>
> Setting a proper TypeSourceInfo to the destructor changes the way it<br>
> it printed (from '~Foo' to '~struct Foo'. Hence the change \
in<br> > DeclarationName.cpp which also fix a bug when printing operator \
names.<br> ><br>
> <a href="http://llvm.org/bugs/show_bug.cgi?id=15125" \
target="_blank">http://llvm.org/bugs/show_bug.<u></u>cgi?id=15125</a><br> <br>
Please add a test with this. Some minor comments below.<br>
<br>
> ---<br>
> lib/AST/DeclarationName.cpp | 5 ++++-<br>
> lib/Sema/SemaExprCXX.cpp | 5 ++++-<br>
> unittests/AST/DeclPrinterTest.<u></u>cpp | 2 +-<br>
> 3 files changed, 9 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/lib/AST/DeclarationName.cpp b/lib/AST/DeclarationName.cpp<br>
> index e064e23..3811bbf 100644<br>
> --- a/lib/AST/DeclarationName.cpp<br>
> +++ b/lib/AST/DeclarationName.cpp<br>
> @@ -537,7 +537,10 @@ void DeclarationNameInfo::<u></u>printName(raw_ostream \
&OS) const {<br> > OS << '~';<br>
> else if (Name.getNameKind() == \
DeclarationName::<u></u>CXXConversionFunctionName)<br> > OS << \
"operator ";<br> > - OS << \
TInfo->getType().getAsString()<u></u>;<br> > + LangOptions LO;<br>
> + PrintingPolicy SubPolicy(LO);<br>
> + SubPolicy.SuppressTagKeyword = true;<br>
> + OS << TInfo->getType().getAsString(<u></u>SubPolicy);<br>
> } else<br>
> OS << Name;<br>
> return;<br>
> diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp<br>
> index a0c123f..7e4f5f3 100644<br>
> --- a/lib/Sema/SemaExprCXX.cpp<br>
> +++ b/lib/Sema/SemaExprCXX.cpp<br>
> @@ -209,7 +209,8 @@ ParsedType Sema::getDestructorName(<u></u>SourceLocation \
TildeLoc,<br> > Context.<u></u>hasSameUnqualifiedType(T, SearchType)) \
{<br> > // We found our type!<br>
><br>
> - return ParsedType::make(T);<br>
> + return CreateParsedType(T, \
Context.<u></u>getTrivialTypeSourceInfo(T,<br> > + \
NameLoc));<br> <br>
This reads better as<br>
<br>
return CreateParsedType(T,<br>
Context.<u></u>getTrivialTypeSourceInfo(T, \
NameLoc));<br> <br>
> }<br>
><br>
> if (!SearchType.isNull())<br>
> @@ -245,6 +246,8 @@ ParsedType Sema::getDestructorName(<u></u>SourceLocation \
TildeLoc,<br> > = \
dyn_cast<<u></u>ClassTemplateSpecializationDec<u></u>l>(Record->getDecl())) \
{<br> > if \
(Spec->getSpecializedTemplate(<u></u>)->getCanonicalDecl() ==<br> > \
Template->getCanonicalDecl())<br> > + return \
CreateParsedType(MemberOfType,<br> > + \
Context.<u></u>getTrivialTypeSourceInfo(<u></u>MemberOfType, NameLoc));<br> <br>
This is a strange way to indent this. According to clang-format, this<br>
should be:<br>
<br>
return CreateParsedType(<br>
MemberOfType,<br>
Context.<u></u>getTrivialTypeSourceInfo(<u></u>MemberOfType, \
NameLoc));<br> <br>
> return ParsedType::make(MemberOfType)<u></u>;<br>
> }<br>
><br>
> diff --git a/unittests/AST/<u></u>DeclPrinterTest.cpp \
b/unittests/AST/<u></u>DeclPrinterTest.cpp<br> > index 44fa742..2e335e3 100644<br>
> --- a/unittests/AST/<u></u>DeclPrinterTest.cpp<br>
> +++ b/unittests/AST/<u></u>DeclPrinterTest.cpp<br>
> @@ -558,7 +558,7 @@ TEST(DeclPrinter, TestCXXConversionDecl3) {<br>
> " operator Z();"<br>
> "};",<br>
> methodDecl(ofClass(hasName("A"<u></u>))).bind("id"),<br>
> - "Z operator struct Z()"));<br>
> + "Z operator Z()"));<br>
> // WRONG; Should be: "operator Z();"<br>
> }<br>
<br>
Looks like the comment here needs updating.<br></blockquote><div><br></div><div>The \
comment is still appropriate: we shouldn't be printing a return type. </div>
_______________________________________________
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