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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
