[PATCH] D18035: [GCC] PR23529 Mangler part of attrbute abi_tag support

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/AST/ItaniumMangle.cpp:272-299
@@ -265,1 +271,30 @@
 
+  // abi_tag is a gcc attribute, taking one or more strings called "tags".
+  //
+  // The goal is to annotate against which version of a library an object was
+  // build and to be able to provide backwards compatibility ("dual abi").
+  //
+  // For this the emitted mangled names have to be different, while you don't
+  // want the user to have to use different names in the source.
+  //
+  // The abi_tag can be present on Struct, Var and Function  declarations as
+  // "explicit" tag, and on inline Namespace as "implicit" tag. Explicit tags
+  // are always emitted after the unqualified name, and (implicit) tags on
+  // namespace are not.
+  //
+  // For functions and variables there is a set of "implicitly available"
+  // tags. These tags are: all tags from the namespace/structs the name is
+  // embedded in, all tags from any template arguments of the name, and, for
+  // functions, alls tags used anywhere in the <bare-function-type> (i.e.
+  // parameters and sometimes the return type).
+  //
+  // For functions this is basically the list of all tags from the signature
+  // without the unqualified name and usually without the return type of the
+  // function. In `operator Type()` Type is NOT part of that list, as it is
+  // part of the unqualified name!
+  //
+  // Now all tags from the function return type/variable type which are not
+  // "implicitly available" must be added to the explicit list of tags, and
+  // are emitted after the unqualified name.
+  //
+  // Example:
----------------
Just reference the documentation for most of this, you don't need to duplicate so much of the description here.

================
Comment at: lib/AST/ItaniumMangle.cpp:275
@@ +274,3 @@
+  // The goal is to annotate against which version of a library an object was
+  // build and to be able to provide backwards compatibility ("dual abi").
+  //
----------------
build -> built

================
Comment at: lib/AST/ItaniumMangle.cpp:321-323
@@ +320,5 @@
+  // track.
+  //
+  // FIXME: how to handle substituted names? They should add the tags used in
+  // the substitution to the list of available tags.
+  class AbiTagState final {
----------------
Do we need to? IIUC, the only time we need this list is when determining the set of "available" tags for a function declaration with a tagged return type, and in that case, a tag can only be available from a substitution if it's also available from the target of that substitution (right?).

================
Comment at: lib/AST/ItaniumMangle.cpp:326-328
@@ +325,5 @@
+    //! All abi tags used implicitly or explicitly
+    std::set<StringRef> UsedAbiTags;
+    //! All explicit abi tags (i.e. not from namespace)
+    std::set<StringRef> EmittedAbiTags;
+
----------------
It seems unnecessarily expensive to hold these as `std::set`s.

================
Comment at: lib/AST/ItaniumMangle.cpp:348-361
@@ +347,16 @@
+
+    void pop() {
+      if (!LinkActive)
+        return;
+
+      assert(LinkHead == this &&
+             "abi tag link head must point to us on destruction");
+      LinkActive = false;
+      if (Parent) {
+        Parent->UsedAbiTags.insert(UsedAbiTags.begin(), UsedAbiTags.end());
+        Parent->EmittedAbiTags.insert(EmittedAbiTags.begin(),
+                                      EmittedAbiTags.end());
+      }
+      LinkHead = Parent;
+    }
+
----------------
Why do we need a stack of these? It seems like we only need one set of available tags for the complete mangling process (it should only be used once at the top level).


http://reviews.llvm.org/D18035