[cfe-commits] r133157 - in /cfe/trunk: lib/CodeGen/CodeGenModule.cpp test/CodeGen/private-extern-redef.c

[Douglas Gregor]

On Jun 16, 2011, at 10:03 AM, jahanian wrote:

> 
> On Jun 16, 2011, at 9:36 AM, Eli Friedman wrote:
> 
>> On Thu, Jun 16, 2011 at 7:49 AM, Fariborz Jahanian <fjahanian at apple.com> wrote:
>>> Author: fjahanian
>>> Date: Thu Jun 16 09:49:42 2011
>>> New Revision: 133157
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=133157&view=rev
>>> Log:
>>> Set the visibility to 'hidden' when previous
>>> declaration of global var is __private_extern__.
>>> // rdar://9609649
>>> 
>>> Added:
>>>   cfe/trunk/test/CodeGen/private-extern-redef.c
>>> Modified:
>>>   cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>>> 
>>> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=133157&r1=133156&r2=133157&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
>>> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Thu Jun 16 09:49:42 2011
>>> @@ -206,8 +206,17 @@
>>> 
>>>  // Set visibility for definitions.
>>>  NamedDecl::LinkageInfo LV = D->getLinkageAndVisibility();
>>> -  if (LV.visibilityExplicit() || !GV->hasAvailableExternallyLinkage())
>>> -    GV->setVisibility(GetLLVMVisibility(LV.visibility()));
>>> +  if (LV.visibilityExplicit() || !GV->hasAvailableExternallyLinkage()) {
>> 
>> If getLinkageAndVisibility is returning the wrong visibility, it
>> should be fixed in the AST/Sema, not hacked around in CodeGen.
> 
> This is not a hack.
> Idea was not to change the AST. It should reflect user source which is one 'hidden' decl.  followed by another
> declaration with default visibility. I can certainly move this code to MergeDecl though. Doug, what do you think?


The AST does perform visibility computations for declarations, which CodeGen is depending on. I suggest checking the computation of visibility for __private_extern__ in the AST is correct (it should handle looking at previous declarations).

	- Doug