On Tue, Jul 7, 2015 at 10:42 AM, Andrey Bokhanko <andreybokha...@gmail.com> wrote: > Aaron, thank you for the review! > > I added a warning (for non applying pragma redefine_extname) and > removed these nasty braces. :-) > > Please re-review: http://reviews.llvm.org/D10805.
> Index: lib/Sema/SemaDecl.cpp > =================================================================== > --- lib/Sema/SemaDecl.cpp > +++ lib/Sema/SemaDecl.cpp > @@ -5561,15 +5561,12 @@ > return true; > } > > -/// \brief Returns true if given declaration is TU-scoped and externally > -/// visible. > -static bool isDeclTUScopedExternallyVisible(const Decl *D) { > +/// \brief Returns true if given declaration has external C language linkage. > +static bool isDeclExternC(const Decl *D) { > if (auto *FD = dyn_cast<FunctionDecl>(D)) > - return (FD->getDeclContext()->isTranslationUnit() || FD->isExternC()) && > - FD->hasExternalFormalLinkage(); > + return FD->isExternC(); > else if (auto *VD = dyn_cast<VarDecl>(D)) > - return (VD->getDeclContext()->isTranslationUnit() || VD->isExternC()) && > - VD->hasExternalFormalLinkage(); > + return VD->isExternC(); > > llvm_unreachable("Unknown type of decl!"); > } > @@ -5998,13 +5995,16 @@ > > NewVD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0), > Context, Label, 0)); > - } else if (!ExtnameUndeclaredIdentifiers.empty() && > - isDeclTUScopedExternallyVisible(NewVD)) { > + } else if (!ExtnameUndeclaredIdentifiers.empty()) { > llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I = > ExtnameUndeclaredIdentifiers.find(NewVD->getIdentifier()); > if (I != ExtnameUndeclaredIdentifiers.end()) { > - NewVD->addAttr(I->second); > - ExtnameUndeclaredIdentifiers.erase(I); > + if (isDeclExternC(NewVD)) { > + NewVD->addAttr(I->second); > + ExtnameUndeclaredIdentifiers.erase(I); > + } else > + Diag(NewVD->getLocation(), diag::warn_redefine_extname_not_applied) > + << 1 << NewVD->getName(); You can leave off the ->getName() part and just pass in the Decl *. Also, can you change 1 into /*Variable*/1? > } > } > > @@ -7526,13 +7526,16 @@ > StringLiteral *SE = cast<StringLiteral>(E); > NewFD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0), > Context, > SE->getString(), 0)); > - } else if (!ExtnameUndeclaredIdentifiers.empty() && > - isDeclTUScopedExternallyVisible(NewFD)) { > + } else if (!ExtnameUndeclaredIdentifiers.empty()) { > llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I = > ExtnameUndeclaredIdentifiers.find(NewFD->getIdentifier()); > if (I != ExtnameUndeclaredIdentifiers.end()) { > - NewFD->addAttr(I->second); > - ExtnameUndeclaredIdentifiers.erase(I); > + if (isDeclExternC(NewFD)) { > + NewFD->addAttr(I->second); > + ExtnameUndeclaredIdentifiers.erase(I); > + } else > + Diag(NewFD->getLocation(), diag::warn_redefine_extname_not_applied) > + << 0 << NewFD->getName(); Same suggestions here. > } > } > > @@ -14355,12 +14358,14 @@ > // 1) declares a function or a variable > // 2) has external linkage > // already exists, add a label attribute to it. > - if (PrevDecl && > - (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl)) && > - PrevDecl->hasExternalFormalLinkage()) > - PrevDecl->addAttr(Attr); > + if (PrevDecl && (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl))) { > + if (isDeclExternC(PrevDecl)) { > + PrevDecl->addAttr(Attr); > + } else Elide the braces. :-) > + Diag(PrevDecl->getLocation(), diag::warn_redefine_extname_not_applied) > + << (isa<FunctionDecl>(PrevDecl) ? 0 : 1) << Name->getName(); And here. :-) > // Otherwise, add a label atttibute to ExtnameUndeclaredIdentifiers. > - else > + } else > (void)ExtnameUndeclaredIdentifiers.insert(std::make_pair(Name, Attr)); > } > > Index: include/clang/Basic/DiagnosticSemaKinds.td > =================================================================== > --- include/clang/Basic/DiagnosticSemaKinds.td > +++ include/clang/Basic/DiagnosticSemaKinds.td > @@ -6333,6 +6333,10 @@ > InGroup<CastQual>, DefaultIgnore; > def warn_cast_qual2 : Warning<"cast from %0 to %1 must have all intermediate > " > "pointers const qualified to be safe">, InGroup<CastQual>, DefaultIgnore; > +def warn_redefine_extname_not_applied : Warning< > + "pragma redefine_extname is applicable to external C declarations only; " > + "not applied to %select{function|variable}0 '%1'">, > + InGroup<Pragmas>; Should be #pragma instead of pragma. If you remove the getName() from above, you can remove the single quotes around %1. The diagnostic emitter will automatically quote things as appropriate then. > } // End of general sema category. > > // inline asm. > Index: test/CodeGen/redefine_extname.c > =================================================================== > --- test/CodeGen/redefine_extname.c > +++ test/CodeGen/redefine_extname.c > @@ -1,4 +1,4 @@ > -// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -w -emit-llvm %s -o - | > FileCheck %s > +// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -Wpragmas -emit-llvm %s -o - > | FileCheck %s > > #pragma redefine_extname fake real > #pragma redefine_extname name alias > @@ -24,3 +24,9 @@ > extern int foo() { return 1; } > // CHECK: define i32 @bar() > > +// Check that pragma redefine_extname applies to external declarations only. > +#pragma redefine_extname foo_static bar_static > +static int foo_static() { return 1; } > +int baz() { return foo_static(); } // expected-warning {{pragma > redefine_extname is applicable to external C declarations only; not applied > to function 'foo_static'}} >From reading this warning, I find the wording slightly confusing because it's talking about the function foo_static on a function call expression, but not on the function definition. I would have expected the diagnostic to follow the declaration, not the expression? > +// CHECK-NOT: call i32 @bar_static() > + > Index: test/CodeGenCXX/redefine_extname.cpp > =================================================================== > --- test/CodeGenCXX/redefine_extname.cpp > +++ test/CodeGenCXX/redefine_extname.cpp > @@ -1,4 +1,4 @@ > -// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -w -emit-llvm %s -o - | > FileCheck %s > +// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -verify -Wpragmas -emit-llvm > %s -o - | FileCheck %s > > extern "C" { > struct statvfs64 { > @@ -20,11 +20,17 @@ > // same name. PR23923. > #pragma redefine_extname foo bar > int f() { > - int foo = 0; > + int foo = 0; // expected-warning {{pragma redefine_extname is applicable > to external C declarations only; not applied to variable 'foo'}} > return foo; > } > extern "C" { > int foo() { return 1; } > // CHECK: define i32 @bar() > } > > +// Check that pragma redefine_extname applies to C code only, and shouldn't > be > +// applied to C++. > +#pragma redefine_extname foo_cpp bar_cpp > +extern int foo_cpp() { return 1; } // expected-warning {{pragma > redefine_extname is applicable to external C declarations only; not applied > to function 'foo_cpp'}} > +// CHECK-NOT: define i32 @bar_cpp() > + > ~Aaron _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits