[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:       Olivier Goffart <ogoffart () kde ! org>
Date:       2014-01-15 22:29:41
Message-ID: 1619145.v4efq9E5jk () finn
[Download RAW message or body]

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.


> 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.

-- 
Olivier

_______________________________________________
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]