[PATCH] Fix source range of destructor name

[Olivier Goffart]

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.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-printing-of-CXX-DeclarationName.patch
Type: text/x-patch
Size: 7132 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140116/ff42bccc/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-source-range-of-the-destructor-name.patch
Type: text/x-patch
Size: 3360 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140116/ff42bccc/attachment-0001.bin>