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. :-)
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