[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-16 0:00:49
Message-ID: 1046194088199062196 () gmail297201516
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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.)
[Attachment #5 (text/html)]
<div>On Wed Jan 15 2014 at 2:29:52 PM, Olivier Goffart <<a \
href="mailto:ogoffart@kde.org">ogoffart@kde.org</a>> wrote:</div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> On Wednesday 15 January 2014 13:50:49 Richard Smith \
wrote:<br> > On Wed, Jan 15, 2014 at 1:25 PM, Olivier Goffart <<a \
href="mailto:ogoffart@kde.org" target="_blank">ogoffart@kde.org</a>> wrote:<br> \
> > Hi,<br> > ><br>
> > This fixes <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> > > \
The problem is that destructor->getNameInfo().<u></u>getSourceRange() only<br> \
> > contains<br> > > the '~' and not the class name after it.<br>
> ><br>
> > This is why for example the destructor name is not highlighted in my \
code<br> > > browser, only the '~' has a tooltip.<br>
> ><br>
> > <a href="http://code.woboq.org/userspace/llvm/tools/clang/include/clang/AST/Declara" \
target="_blank">http://code.woboq.org/<u></u>userspace/llvm/tools/clang/<u></u>include/clang/AST/Declara</a><br>
> > tionName.h.html#_<u></u>ZN5clang20Declarationisn'<u></u>tNameTableD1Ev<<a \
href="http://code.woboq" target="_blank">http://code.<u></u>woboq</a><br> > > \
.org/userspace/llvm/tools/<u></u>clang/include/clang/AST/<u></u>DeclarationName.h.html#_<br>
> > ZN5clang20DeclarationNameTable<u></u>D1Ev><br>
> + return CreateParsedType(MemberOfType,<br>
> + \
Context.<u></u>getTrivialTypeSourceInfo(<u></u>MemberOfType,<br> > NameLoc));<br>
> return ParsedType::make(MemberOfType)<u></u>;<br>
><br>
> Please delete the unreachable 'return' statement here.<br>
<br>
This is obviously a left over.<br>
<br>
><br>
> There are 3 other 'return ParsedType::make(...);' calls in this \
function,<br> > and they all seem to have the same issue; do these need changes \
too?<br> <br>
Right, I'll change them too.<br>
<br>
> Also, you aren't including the location information from the CXXScopeSpec \
in<br> > the produced TypeSourceInfo, so this still doesn't look exactly right \
(for<br> > p->X::Y::~Y(), you probably won't be able to provide an \
appropriate tooltip<br> > for the 'X', even with this fix, for \
instance).<br> <br>
I fail to understand this.<br>
<br>
only ~Y here is the name of the destructor.<br>
If I am not mistaken, X::Y:: is the qualifier (as traversed with destructor-<br>
>getQualifierLoc()) and should not be part of the function name \
itself.<br></blockquote><div><br></div><div>Sorry, yes, you're absolutely \
right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">
> The changes to DeclarationName.cpp and to DeclPrinterTest.cpp look<br>
> unrelated to this fix (right?), and the fix itself seems to be missing a<br>
> test.<br>
<br>
They are related because now that the destructor has a full type information,<br>
'~class Foo' is written instead of '~Foo' and other tests were \
failing<br> without that change.<br></blockquote><div><br></div><div>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.</div> \
<div><br></div><div>(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.)</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