[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

Sat Jul 8 13:01:27 PDT 2023

MaskRay added a comment.

In D153835#4481816 <https://reviews.llvm.org/D153835#4481816>, @rjmccall wrote:

> I agree that the change to test51 is the right result, yes.  The explicit visibility attribute on the template declaration should take precedence over the visibility of the types in the template parameter list, and then the explicit visibility attribute on the variables should take precedence over the visibility of the variable types, and then the instantiation should be a specialization of a default-visibility template with default-visibility arguments.  But I think that logic ought to be fixed in `getLVFor...`, not by changing the basic propagation of attributes.
>
>> I actually think that the patch makes VisibilityAttr behave in a more normal way as the majority of attributes are cloned by instantiateTemplateAttribute in the tablegen-generated tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc.
>
> You're not really just cloning it, because making the cloned attribute implicit (i.e. giving it the effect of a visibility pragma instead of an attribute) is a huge difference.

OK, created D154774 <https://reviews.llvm.org/D154774> as an alternative. I added more cases to test51 and test71. D154774 <https://reviews.llvm.org/D154774> will change the tests in the same way that this VisibilityAttr cloning patch changes the tests.

I did not know that implicit VisibilityAttr is for `#pragma GCC visibility push(...)`. I agree that reusing `IsImplicit` for an instantiated `FunctionTemplateDecl/CXXMethodDecl` has unclear interaction.

However, I am not fully convinced that avoiding cloning `VisibilityAttr` is the most clean way forward. That said, since D154774 <https://reviews.llvm.org/D154774> will have the same effect as far as `CodeGenCXX/visibility.cpp` is concerned, I am fine not pursing this patch forward.
I wonder whether using another bit that is not `IsImplicit` will be better.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153835/new/

https://reviews.llvm.org/D153835