LGTM, but you should wait for Richard to weigh in as well. Thank you!
~Aaron On Wed, Jul 8, 2015 at 9:36 AM, Andrey Bokhanko <andreybokha...@gmail.com> wrote: > 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