[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 <richard () metafoo ! co ! uk>
Date:       2014-01-15 21:50:49
Message-ID: CAOfiQqmG6fAoFQTyWqG=qc2+RU4qs3PEZF6PSeL7cZVGWkjUPQ () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]


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

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

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.


[Attachment #5 (text/html)]

<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 15, 2014 \
at 1:25 PM, Olivier Goffart <span dir="ltr">&lt;<a href="mailto:ogoffart@kde.org" \
target="_blank">ogoffart@kde.org</a>&gt;</span> wrote:<br> <blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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-&gt;getNameInfo().getSourceRange() only contains<br> the &#39;~&#39; \
and not the class name after it.<br> <br>
This is why for example the destructor name is not highlighted in my code<br>
browser, only the &#39;~&#39; has a tooltip.<br>
<a href="http://code.woboq.org/userspace/llvm/tools/clang/include/clang/AST/DeclarationName.h.html#_ZN5clang20DeclarationNameTableD1Ev" \
target="_blank">http://code.woboq.org/userspace/llvm/tools/clang/include/clang/AST/Dec \
larationName.h.html#_ZN5clang20Declarationisn&#39;tNameTableD1Ev</a></blockquote> \
<div><br></div><div>+            return \
CreateParsedType(MemberOfType,<br></div><div><div>+                    \
Context.getTrivialTypeSourceInfo(MemberOfType, NameLoc));</div><div>             \
return ParsedType::make(MemberOfType);</div> </div><div><br></div><div>Please delete \
the unreachable &#39;return&#39; statement here.</div><div><br></div><div>There are 3 \
other &#39;return ParsedType::make(...);&#39; calls in this function, and they all \
seem to have the same issue; do these need changes too? Also, you aren&#39;t \
including the location information from the CXXScopeSpec in the produced \
TypeSourceInfo, so this still doesn&#39;t look exactly right (for p-&gt;X::Y::~Y(), \
you probably won&#39;t be able to provide an appropriate tooltip for the &#39;X&#39;, \
even with this fix, for instance).</div> <div><br></div><div>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.</div></div></div></div>



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