<div dir="ltr">Both patches LGTM, thanks!</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 16, 2014 at 4:13 AM, Olivier Goffart <span dir="ltr"><<a href="mailto:ogoffart@kde.org" target="_blank">ogoffart@kde.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 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 attached<br>
type location. With a test this time.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Olivier<br>
</font></span><div class="HOEnZb"><div class="h5"><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">ogoffart@kde.org</a>> 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">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.cgi?id=15125</a><br>
> > > > The problem is that destructor->getNameInfo().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<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/userspace/llvm/tools/clang/</a><br>
> ><br>
> > include/clang/AST/Declara<br>
> ><br>
> > > > tionName.h.html#_ZN5clang20Declarationisn'tNameTableD1Ev<<a href="http://code" target="_blank">http://code</a>.<br>
> ><br>
> > woboq<br>
> ><br>
> > > > .org/userspace/llvm/tools/clang/include/clang/AST/<br>
> ><br>
> > DeclarationName.h.html#_<br>
> ><br>
> > > > ZN5clang20DeclarationNameTableD1Ev><br>
> > ><br>
> > > +            return CreateParsedType(MemberOfType,<br>
> > > +                    Context.getTrivialTypeSourceInfo(MemberOfType,<br>
> > > NameLoc));<br>
> > ><br>
> > >              return ParsedType::make(MemberOfType);<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 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 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 missing a<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 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>
</div></div></blockquote></div><br></div>