<div dir="ltr">On 31 January 2013 19:28, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<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"><div><div>On Thu, Jan 31, 2013 at 6:34 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>> wrote:<br>
> On 30 January 2013 22:43, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> SortUndefinedButUsed is not a strict weak ordering. Consider:<br>
>><br>
>> A = {1, invalid}<br>
>> B = {2, 1}<br>
>> C = {0, 2}<br>
>><br>
>> We have A < B (comparing first because A.second is invalid), B < C<br>
>> (comparing second), and C < A (comparing first because A.second is<br>
>> invalid).<br>
><br>
><br>
> Fixed. Thanks for catching that.<br>
><br>
>> @@ -8428,12 +8436,12 @@<br>
>> if (FD) {<br>
>> FD->setBody(Body);<br>
>><br>
>> - // The only way to be included in UndefinedInternals is if there is<br>
>> an<br>
>> + // The only way to be included in UndefinedButUsed is if there is an<br>
>> // ODR-use before the definition. Avoid the expensive map lookup if<br>
>> this<br>
>> // is the first declaration.<br>
>> - if (FD->getPreviousDecl() != 0 && FD->getPreviousDecl()->isUsed() &&<br>
>> - FD->getLinkage() != ExternalLinkage)<br>
>> - UndefinedInternals.erase(FD);<br>
>> + if (FD->getPreviousDecl() != 0 && FD->getPreviousDecl()->isUsed())<br>
>> + if (FD->getLinkage() != ExternalLinkage && !FD->isInlined())<br>
>> + UndefinedButUsed.erase(FD);<br>
>><br>
>> Shouldn't this be (FD->getLinkage() != ExternalLinkage ||<br>
>> FD->getPreviousDecl()->isInlined()) ?<br>
><br>
><br>
> Right.<br>
><br>
>> +namespace test7 {<br>
>> + // It is invalid to redeclare a function inline after its definition,<br>
>> but we<br>
>> + // do not diagnose this yet.<br>
>> + void f(); // expected-warning{{inline function 'test7::f' is not<br>
>> defined}}<br>
>> + void test() { f(); } // no used-here note.<br>
>> + inline void f();<br>
>> +}<br>
>><br>
>> This test doesn't redeclare a function inline after its definition,<br>
>> just after its use.<br>
><br>
><br>
> Erp, of course. Removed the stale comment.<br>
><br>
>> On Wed, Jan 30, 2013 at 10:19 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>><br>
>> wrote:<br>
>> > This patch extends the tracking for used but undefined functions and<br>
>> > variables with internal linkage to also apply to inline functions.<br>
>> > Sema::UndefinedInternals is renamed Sema::UndefinedButUsed.<br>
>> ><br>
>> > Fixes PR14993. Please review! Please pay particular attention to the<br>
>> > behaviour of the error under C99 and GNU89 modes.<br>
>><br>
>> I've not looked at this from the perspective of C yet, but it looks<br>
>> correct for C++ (modulo the above comments).<br>
><br>
><br>
> C99 has a similar rule:<br>
><br>
> "For a function with external linkage, the following restrictions apply:<br>
> If a function is declared with an inline function specifier, then it shall<br>
> also be defined in the same translation unit."<br>
> - C99 6.7.4p6<br>
><br>
> Of course, a function with internal linkage and isn't covered by the above,<br>
> but it doesn't matter since that's invalid for other reasons. Good news, in<br>
> C99 mode GCC warns on declaration without definition of 'inline', 'extern<br>
> inline' functions, and used but undefined 'static inline' functions.<br>
><br>
> Bad news, GNU89 mode seems to permit it. 'static inline' functions get the<br>
> same warning as in C99 mode, but in practice you can define a function of<br>
> the same name in another TU, without 'static inline', and it links fine.<br>
> With 'extern inline' I think it's even deliberate.<br>
><br>
> I've changed the patch to simply not warn on used but not defined<br>
> gnu89-inline functions.<br>
<br>
OK, fair enough.<br>
<br>
<br>
@@ -366,13 +366,17 @@<br>
}<br>
<br>
namespace {<br>
- struct SortUndefinedInternal {<br>
+ struct SortUndefinedButUsed {<br>
const SourceManager &SM;<br>
- explicit SortUndefinedInternal(SourceManager &SM) : SM(SM) {}<br>
+ explicit SortUndefinedButUsed(SourceManager &SM) : SM(SM) {}<br>
<br>
bool operator()(const std::pair<NamedDecl *, SourceLocation> &l,<br>
const std::pair<NamedDecl *, SourceLocation> &r) const {<br>
- if (l.second != r.second)<br>
+ if (l.second.isValid() && !r.second.isValid())<br>
+ return true;<br>
+ if (!l.second.isValid() && r.second.isValid())<br>
+ return false;<br>
+ if (l.second != r.second && l.second.isValid() && r.second.isValid())<br>
<br>
I think you can drop the && l.second.isValid() && r.second.isValid()<br>
here -- invalid source locations compare equal.<br></div></div></blockquote><div><br></div><div>Done.</div><div><br></div><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">
<div><div>+++ lib/Sema/SemaDecl.cpp (working copy)<br>
@@ -2215,6 +2215,14 @@<br>
New->setType(QualType(NewType, 0));<br>
NewQType = Context.getCanonicalType(New->getType());<br>
}<br>
+<br>
+ // If this redeclaration makes the function inline, we may need to add it to<br>
+ // UndefinedButUsed.<br>
+ if (!Old->isInlined() && New->isInlined() &&<br>
+ Old->isUsed(false) &&<br>
+ !Old->isDefined() && !New->isThisDeclarationADefinition())<br>
+ UndefinedButUsed.insert(std::make_pair(Old->getCanonicalDecl(),<br>
+ SourceLocation()));<br>
<br>
Do you need a GNU inline check here?<br></div></div></blockquote><div><br></div><div>Yep.</div><div><br></div><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">
<div><div>Also, what happens if a redeclaration adds the gnu_inline attribute to<br>
a function which is already declared inline? In g++ that's an error,<br>
but Clang appears to allow it.<br></div></div></blockquote><div><br></div><div>Yep again. See the new tests!</div><div><br></div><div>Nick</div><div><br></div><div><br></div></div></div></div>