[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-22 0:35:39
Message-ID: -6798167422057077741 () gmail297201516
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Mon Jan 20 2014 at 7:55:19 AM, Olivier Goffart <ogoffart@kde.org> wrote:
> On Thursday 16 January 2014 16:25:58 Richard Smith wrote:
> > Both patches LGTM, thanks!
>
> Could you please commit them as I do not have SVN access.
>
Committed as r199778 and 199779, thanks!
> --
> Thanks.
>
> >
> > On Thu, Jan 16, 2014 at 4:13 AM, Olivier Goffart <ogoffart@kde.org>
> wrote:
> > > 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.)
>
>
[Attachment #5 (text/html)]
<div>On Mon Jan 20 2014 at 7:55:19 AM, 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 Thursday 16 January 2014 16:25:58 Richard Smith \
wrote:<br> > Both patches LGTM, thanks!<br>
<br>
Could you please commit them as I do not have SVN \
access.<br></blockquote><div><br></div><div>Committed as r199778 and 199779, \
thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">
--<br>
Thanks.<br>
<br>
><br>
> On Thu, Jan 16, 2014 at 4:13 AM, Olivier Goffart <<a \
href="mailto:ogoffart@kde.org" target="_blank">ogoffart@kde.org</a>> wrote:<br> \
> > Hi,<br> > ><br>
> > Thanks for the feed back.<br>
> > I've now split the patch into two. The first one fix the printing of \
the<br> > > tag<br>
> > name, when printing DeclarationName.<br>
> > There was one more way to print declaration name, and i think they \
should<br> > > follow the same pattern.<br>
> > And the second patch fix the issue that the destructor did not have an<br>
> > attached<br>
> > type location. With a test this time.<br>
> ><br>
> > --<br>
> > Olivier<br>
> ><br>
> > On Thursday 16 January 2014 00:00:49 Richard Smith wrote:<br>
> > > On Wed Jan 15 2014 at 2:29:52 PM, Olivier Goffart <<a \
href="mailto:ogoffart@kde.org" target="_blank">ogoffart@kde.org</a>><br> > \
><br> > > wrote:<br>
> > > > 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>><br> > > \
> ><br> > > > > 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()<br> > > > > > \
> 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<br> > ><br>
> > my<br>
> ><br>
> > > > code<br>
> > > ><br>
> > > > > > browser, only the '~' has a tooltip.<br>
> > > > > ><br>
> > > > > > <a \
href="http://code.woboq.org/userspace/llvm/tools/clang/" \
target="_blank">http://code.woboq.org/<u></u>userspace/llvm/tools/clang/</a><br> > \
> > ><br> > > > > include/clang/AST/Declara<br>
> > > ><br>
> > > > > > \
tionName.h.html#_<u></u>ZN5clang20Declarationisn'<u></u>tNameTableD1Ev<<br> \
> ><br> > > <a href="http://code" target="_blank">http://code</a>.<br>
> ><br>
> > > > woboq<br>
> > > ><br>
> > > > > > \
.org/userspace/llvm/tools/<u></u>clang/include/clang/AST/<br> > > > ><br>
> > > > DeclarationName.h.html#_<br>
> > > ><br>
> > > > > > ZN5clang20DeclarationNameTable<u></u>D1Ev><br>
> > > > ><br>
> > > > > + return CreateParsedType(MemberOfType,<br>
> > > > > + \
Context.<u></u>getTrivialTypeSourceInfo(<u></u>MemberOfType,<br> > > > > \
> NameLoc));<br> > > > > ><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>
> > > > > There are 3 other 'return ParsedType::make(...);' \
calls in this<br> > > > > > 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<br> > > > ><br>
> > > > CXXScopeSpec in<br>
> > > ><br>
> > > > > the produced TypeSourceInfo, so this still doesn't look \
exactly<br> > > > > > right<br>
> > > ><br>
> > > > (for<br>
> > > ><br>
> > > > > p->X::Y::~Y(), you probably won't be able to provide \
an appropriate<br> > > > ><br>
> > > > tooltip<br>
> > > ><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<br> > > > > destructor-<br>
> > > ><br>
> > > > >getQualifierLoc()) and should not be part of the function \
name<br> > ><br>
> > itself.<br>
> ><br>
> > > Sorry, yes, you're absolutely right.<br>
> > ><br>
> > > > > The changes to DeclarationName.cpp and to \
DeclPrinterTest.cpp look<br> > > > > > unrelated to this fix (right?), \
and the fix itself seems to be<br> > ><br>
> > missing a<br>
> ><br>
> > > > > test.<br>
> > > ><br>
> > > > They are related because now that the destructor has a full \
type<br> > > > > information,<br>
> > > ><br>
> > > > '~class Foo' is written instead of '~Foo' and \
other tests were<br> > ><br>
> > failing<br>
> ><br>
> > > > without that change.<br>
> > ><br>
> > > OK, it's a shame that we need to do this, but this looks fine. \
It'd be a<br> > > > little nicer to set LangOpts.CPlusPlus rather than \
setting<br> > > > SuppressTagKeywords.<br>
> > ><br>
> > > (I wonder if we can remove the printing of the tag keyword from<br>
> > > TypePrinter::printTag -- it should be wrapped in an ElaboratedType \
if<br> > > > that's necessary, and the tag keyword would get printed \
there.)<br> <br>
</blockquote>
_______________________________________________
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