[clang] Document the type_visibility attribute

[ldionne]

I was looking for the documentation of that attribute, and the best I could find was a Stackoverflow answer or the commit message that originally introduced the attribute. I figured I might as well document what I find to save everyone time in the future.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Louis Dionne (ldionne)

Changes

I was looking for the documentation of that attribute, and the best I could find was a Stackoverflow answer or the commit message that originally introduced the attribute. I figured I might as well document what I find to save everyone time in the future.

---

Full diff: https://github.com/llvm/llvm-project/pull/79157.diff

2 Files Affected:

• (modified) clang/include/clang/Basic/Attr.td (+2-2)
• (modified) clang/include/clang/Basic/AttrDocs.td (+15)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 58838b01b4fd7ca..e47107d8edcb378 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3220,8 +3220,8 @@ def TypeVisibility : InheritableAttr {
   let Args = [EnumArgument<"Visibility", "VisibilityType",
                            ["default", "hidden", "internal", "protected"],
                            ["Default", "Hidden", "Hidden", "Protected"]>];
-//  let Subjects = [Tag, ObjCInterface, Namespace];
-  let Documentation = [Undocumented];
+  let Subjects = SubjectList<[Tag, ObjCInterface, Namespace], ErrorDiag>;
+  let Documentation = [TypeVisibilityDocs];
 }
 
 def VecReturn : InheritableAttr {
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 7e633f8e2635a9a..703f7cd314b727c 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -5557,6 +5557,21 @@ See :doc:`LTOVisibility`.
   }];
 }
 
+def TypeVisibilityDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+The ``type_visibility`` attribute allows the ELF visibility of a type and its vague
+linkage objects (vtable, typeinfo, typeinfo name) to be controlled separately from
+the visibility of functions and data members of the type.
+
+For example, this can be used to give default visibility to the typeinfo and the vtable
+of a type while still keeping hidden visibility on its member functions and static data
+members.
+
+This attribute can only be applied to types and namespaces.
+  }]
+}
+
 def RenderScriptKernelAttributeDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{

[ldionne]

I'm not certain if this is correct, and whether this means I need to add some tests.

[AaronBallman]

I would separate out this change because it's not really related to documenting the attribute (this is used to generate automatic checks for the subject, so it corresponds to this code that was manually written and can possibly be removed:

llvm-project/clang/lib/Sema/SemaDeclAttr.cpp

Line 2917 in 091fc81

if (isTypeVisibility && !(isa<TagDecl>(D) || isa<ObjCInterfaceDecl>(D) ||

)

[philnik777]

Maybe we also want to document the behavior of adding both clang::type_visibility and gnu::visibility to a namespace? (I think clang::type_visibility overrides gnu::visibility)

[ldionne]

You seem to be right based on https://godbolt.org/z/bbs7478sv. I'll add the documentation.

[ldionne]

This applies to classes too: https://godbolt.org/z/673qsnxsP

[AaronBallman]

Thank you for the improved documentation! In general, heading in the right direction. There might be some more details we'd like to add, but if you don't know those details, that's totally fine (this at least moves the needle in the right direction).

[AaronBallman]

I would separate out this change because it's not really related to documenting the attribute (this is used to generate automatic checks for the subject, so it corresponds to this code that was manually written and can possibly be removed:

llvm-project/clang/lib/Sema/SemaDeclAttr.cpp

Line 2917 in 091fc81

if (isTypeVisibility && !(isa<TagDecl>(D) || isa<ObjCInterfaceDecl>(D) ||

)

[AaronBallman]

Does this only impact ELF or does this also impact say PE32?

[ldionne]

I don't know the answer to that question since I have no familiarity with PE32. However, looking at the original patch it looks to me like this impacts all places where a normal visibility attribute is relevant. I think I should just remove ELF here from the docs, since this makes it look unnecessarily specific to ELF.

[AaronBallman]

LGTM, thank you for the improved documentation!