On Wed, Jul 8, 2015 at 5:50 PM, Andrey Bokhanko <andreybokha...@gmail.com> wrote: > Aaron, thank you for investing your time for reviews! -- your comments > give me a good run for my money, but patches definitely become better > in result. :-)
My pleasure! :-) ~Aaron > > Yours, > Andrey > > > On Wed, Jul 8, 2015 at 9:48 PM, Aaron Ballman <aaron.ball...@gmail.com> wrote: >> 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