On Wed, Jun 24, 2015 at 6:41 AM, Andrey Bokhanko <[email protected]> wrote: > Patch updated in response to Aaron Ballman's comments > (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/131538.html).
Getting there! A few more nits and a question below. > > > http://reviews.llvm.org/D10646 > > Files: > lib/Sema/SemaDecl.cpp > test/CodeGen/redefine_extname.c > > Index: test/CodeGen/redefine_extname.c > =================================================================== > --- test/CodeGen/redefine_extname.c > +++ test/CodeGen/redefine_extname.c > @@ -13,3 +13,14 @@ > // CHECK: call i32 @real() > // Check that this also works with variables names > // CHECK: load i32, i32* @alias > + > +// This is a case when redefenition is deferred *and* we have a local of the > +// same name. PR23923. > +#pragma redefine_extname foo bar > +int f() { > + int foo = 0; > + return foo; > +} > +extern int foo() { return 1; } > +// CHECK: define i32 @bar() Should this also work with code like: #pragma redefine_extname foo bar int f() { int foo = 0; return foo; } extern "C" { int foo() { return 1; } } If so, I'd like to see a test case for that. > + > Index: lib/Sema/SemaDecl.cpp > =================================================================== > --- lib/Sema/SemaDecl.cpp > +++ lib/Sema/SemaDecl.cpp > @@ -5522,6 +5522,18 @@ > return true; > } > > +/// \brief Returns true if given declaration is TU-scoped and externally > visible Missing a period at the end of the comment. > +static bool isDeclTUScopedExternallyVisible(Decl *Decl) { The Decl passed in can be const. Also, please name the identifier something other than the type. D would suffice. > + if (auto *FD = dyn_cast<FunctionDecl>(Decl)) > + return (FD->getDeclContext()->isTranslationUnit() || FD->isExternC()) && > + FD->hasExternalFormalLinkage(); > + else if (auto *VD = dyn_cast<VarDecl>(Decl)) > + return (VD->getDeclContext()->isTranslationUnit() || VD->isExternC()) && > + VD->hasExternalFormalLinkage(); > + else > + llvm_unreachable("Unknown type of decl!"); I would drop the else and put the llvm_unreachable as the last statement in the function. > +} > + > NamedDecl * > Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC, > TypeSourceInfo *TInfo, LookupResult &Previous, > @@ -5946,7 +5958,8 @@ > > NewVD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0), > Context, Label, 0)); > - } else if (!ExtnameUndeclaredIdentifiers.empty()) { > + } else if (!ExtnameUndeclaredIdentifiers.empty() && > + isDeclTUScopedExternallyVisible(NewVD)) { > llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I = > ExtnameUndeclaredIdentifiers.find(NewVD->getIdentifier()); > if (I != ExtnameUndeclaredIdentifiers.end()) { > @@ -7468,7 +7481,8 @@ > StringLiteral *SE = cast<StringLiteral>(E); > NewFD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0), > Context, > SE->getString(), 0)); > - } else if (!ExtnameUndeclaredIdentifiers.empty()) { > + } else if (!ExtnameUndeclaredIdentifiers.empty() && > + isDeclTUScopedExternallyVisible(NewFD)) { > llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I = > ExtnameUndeclaredIdentifiers.find(NewFD->getIdentifier()); > if (I != ExtnameUndeclaredIdentifiers.end()) { > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
