[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/Sema/AttributeList.h:601
+      sizeof(AttributeList)
+      + (sizeof(DeclSpecUuidDecl) + sizeof(void *) + sizeof(ArgsUnion) - 1)
         / sizeof(void*) * sizeof(void*)
----------------
You're not storing a `DeclSpecUuidDecl` within the attribute, so you don't need to reserve enough space for one here. In fact, this constant appears to be unused, so you can just remove it.


================
Comment at: lib/AST/Decl.cpp:4561
+
+DeclSpecUuidDecl * DeclSpecUuidDecl::Create(const ASTContext &C, DeclContext *DC,
+                                            SourceLocation IdLoc,
----------------
No space after `*`, please.


================
Comment at: lib/AST/Decl.cpp:4564
+                                            StringRef UuidVal)
+{
+  return new (C, DC) DeclSpecUuidDecl(DeclSpecUuid, DC, IdLoc, UuidVal);
----------------
`{` on the previous line, please.


================
Comment at: lib/AST/DeclCXX.cpp:1727
       ((getName() == "IUnknown" &&
-        Uuid->getGuid() == "00000000-0000-0000-C000-000000000046") ||
+        Uuid->getDeclSpecUuidDecl()->getStrUuid() == "00000000-0000-0000-C000-000000000046") ||
        (getName() == "IDispatch" &&
----------------
It would be nice to have a convenience accessor on `UuidAttr` to get the UUID as a string. (You can add one using `AdditionalMembers` in Attr.td.)


================
Comment at: lib/Parse/ParseDecl.cpp:568-583
+    // Clean up the string from the "\" at begining and at end.
+    StringRef UuidStr1 = UuidStr.ltrim('\"');
+    StringRef TrimmedUuidStr = UuidStr1.rtrim('\"');
+    DeclSpecUuidDecl *ArgDecl =
+      DeclSpecUuidDecl::Create(Actions.getASTContext(),
+                               Actions.getFunctionLevelDeclContext(),
+                               SourceLocation(),
----------------
The `Parser` should not create `Decl`s (that's a layering violation). There are two ways to handle this:

1) Add a `Sema` method to act on a UUID attribute string, and return a pointer that you can stash in the `Attrs` list.
2) Just store a string in the `Attrs` list, and move the code to convert the string into a `DeclSpecUuidDecl*` and store that on the `UuidAttr` into `Sema`.

I'd strongly prefer the second option; from the point of view of the parser, we can still just treat this as an attribute that takes a string, and we can do the string -> `DeclSpecUuidDecl` handling entirely within the semantic analysis for the attribute (in `Sema`).


================
Comment at: lib/Parse/ParseDecl.cpp:572
+    DeclSpecUuidDecl *ArgDecl =
+      DeclSpecUuidDecl::Create(Actions.getASTContext(),
+                               Actions.getFunctionLevelDeclContext(),
----------------
What should happen if multiple declarations (of distinct entities) use the same `__declspec(uuid(...))` attribute? Should you get a redefinition error for the attribute or should they all share the same UUID entity? Either way, we'll want to do some (minimal) UUID lookup and build a redeclaration chain for `DeclSpecUuidDecl`s.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:4937-4938
-      return nullptr;
-    Diag(UA->getLocation(), diag::err_mismatched_uuid);
-    Diag(Range.getBegin(), diag::note_previous_uuid);
-    D->dropAttr<UuidAttr>();
----------------
Do we still diagnose UUID mismatches somewhere else?


https://reviews.llvm.org/D43576