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
undefined-but-used-3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
