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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D153835#4478700 <https://reviews.llvm.org/D153835#4478700>, @efriedma wrote:

> Can you write the complete rule we're trying to follow here?  Like, if you have a free function template, prioritize attributes in the following order ..., if you have a member function, use the following order ..., etc.  I know that's a significant amount of writing, but I don't really have any intuition here about how it should work. So I'm not sure how to review this without a reference.  (The closest thing I have to intuition is comparing the rules to the ones for C++ anonymous namespaces, but visibility is a lot more complicated.)

The rules are quite complex and sorry that I am unable to describe them...

> Given how complicated the rules are here, it might be easier to understand if you teach the relevant code in clang/lib/AST/Decl.cpp (getExplicitVisibilityAux, I think?) to explicitly check for attributes on the original template, instead of cloning the attribute.

If we don't clone `VisibilityAttr`, the best I can come up with is the following patch

  --- a/clang/lib/AST/Decl.cpp
  +++ b/clang/lib/AST/Decl.cpp
  @@ -375,6 +375,9 @@ static bool shouldConsiderTemplateVisibility(const FunctionDecl *fn,
     if (!specInfo->isExplicitInstantiationOrSpecialization())
       return true;
  
  +  if (specInfo->getTemplate()->getTemplatedDecl()->hasAttr<VisibilityAttr>())
  +    return false;
  +
     return !fn->hasAttr<VisibilityAttr>();
   }

test71 still fails (while this patch fixes test71). Here is my observation:

As confirmed by `-fsyntax-only -Xclang -ast-dump`, the explicit instantiation declaration (`extern template struct DEFAULT foo<int>;`) clones the `FunctionTemplateDecl` tree (including `CXXMethodDecl`) without cloning `VisibilityAttr`. This causes all `hasAttr<VisibilityAttr>()` code to behave incorrectly like the status quo.

I think this `VisibilityAttr` cloning patch matches GCC's spirit better and will enable simplification of the complex template instantiation/specialization rules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153835