[PATCH] Fix source range of destructor name

[Olivier Goffart]

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.

-- 
Thanks.

> 
> On Thu, Jan 16, 2014 at 4:13 AM, Olivier Goffart <ogoffart at 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 at 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 at 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.)