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