Aaron, thank you! -- I fixed everything; please re-review: http://reviews.llvm.org/D10805
Yours, Andrey On Tue, Jul 7, 2015 at 6:41 PM, Aaron Ballman <aaron.ball...@gmail.com> wrote: > 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