MaskRay added a subscriber: erichkeane. MaskRay added a comment. In D153835#4481462 <https://reviews.llvm.org/D153835#4481462>, @rjmccall wrote:
> We made a decision ten years or so ago to use a slightly different > interpretation of the visibility attributes, so differences from GCC are not > by themselves unexpected or problematic. Sure, I think we should assess every corner case and decide whether it makes sense to match GCC compatibility. I started the patch by noticing https://github.com/llvm/llvm-project/issues/31462 (D29157 <https://reviews.llvm.org/D29157>) to address a specific libc++ problem, not that I started to match a random GCC behavior. I have noticed that libcxx/include/__config:592 says // The inline should be removed once PR32114 is resolved # define _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS inline _LIBCPP_HIDDEN (The doc is old stale. This is something I want to improve next.) > In this case, I'm not sure I agree that it's a bug that an attribute on an > explicit class template instantiation overrides attributes on member > functions in the template pattern. OK, I think this is whether our opinions differ. I think that: - (test51)(consensus bugfix?) the visibility attribute in `template<foo *z> void DEFAULT zed()` should affect template void zed<&x>(); - (test71)(differed opinion) the visibility attribute in `template <class U> HIDDEN U bar();` should override the visibility attribute in `extern template struct DEFAULT foo<int>;`, like GCC does, as the method attribute is more specific. Perhaps #libc <https://reviews.llvm.org/tag/libc/> folks and @erichkeane (who introduced `MeaningfulToClassTemplateDefinition`) can weigh in. > Even if we decide to adopt a new rule here, though, I agree with Eli that > this seems like a very heavy-handed way of implementing it; basically every > line in this patch seems to be impactful in ways that are hard to anticipate. 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`. However, `MeaningfulToClassTemplateDefinition` attributes are rare and even few need to think about precedence for class attributes and method attributes. This does make VisibilityAttr unusual, but I think the complexity resides mainly in the existing code, not the code I added in this patch. 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
