[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/AST/Decl.h:4303
+
+   StringLiteral *getSTLUuid() { return STLUuid; }
+};
----------------
What does "STL" mean here?


================
Comment at: include/clang/Sema/ParsedAttr.h:337-346
+  /// Constructor for __declspec(uuid) attribute.
+  ParsedAttr(IdentifierInfo *attrName, SourceRange attrRange,
+             IdentifierInfo *scopeName, SourceLocation scopeLoc,
+             StringLiteral *stluuid, ParsedAttr::Syntax syntaxUsed)
+      : AttrName(attrName), ScopeName(scopeName), AttrRange(attrRange),
+        ScopeLoc(scopeLoc), Invalid(false), HasParsedType(false),
+    SyntaxUsed(syntaxUsed), NumArgs(1) {
----------------
I don't think we need a custom constructor for this any more; a `StringLiteral` is an `Expr`, so can be stored as a regular argument. It's also suspicious that this constructor doesn't use its `stluuid` parameter -- maybe it's already unused?


================
Comment at: include/clang/Sema/ParsedAttr.h:822
+  ParsedAttr *
+  createUuidDeclSpecAttribute(IdentifierInfo *attrName, SourceRange attrRange,
+                              IdentifierInfo *scopeName, SourceLocation scopeLoc,
----------------
(Likewise we shouldn't need this any more...)


================
Comment at: include/clang/Sema/ParsedAttr.h:1031
+  ParsedAttr *
+  addNew(IdentifierInfo *attrName, SourceRange attrRange, IdentifierInfo *scopeName,
+         SourceLocation scopeLoc, StringLiteral *stluuid, ParsedAttr::Syntax syntaxUsed) {
----------------
(... or this.)


================
Comment at: include/clang/Sema/Sema.h:408
 
+  std::map<const Decl*, DeclSpecUuidDecl*> DeclToDeclSpecUuidDecl;
+
----------------
This map is not necessary (you can get the `DeclSpecUuidDecl` from the `UuidAttr` on the `Decl`) and not correct (it's not serialized and deserialized). Please remove it.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:1071-1073
+  const auto ExistingRecord = Manglings.find(MangledName);
+  if (ExistingRecord != std::end(Manglings))
+    Manglings.remove(&(*ExistingRecord));
----------------
Was this supposed to be included in this patch? It looks like this is papering over a bug elsewhere.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5405-5408
+    if (UA->getDeclSpecUuidDecl()->getSTLUuid()->getString().equals_lower(
+            Uuid->getSTLUuid()->getString()) &&
+        declaresSameEntity(DeclToDeclSpecUuidDecl.find(D)->first, D))
       return nullptr;
----------------
This `if` should be:

```
if (declaresSameEntity(UA->getDeclSpecUuidDecl(), Uuid))
```

(Not a string comparison. We already combined `UuidDecl`s with the same string into the same entity.)


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5452-5460
+  QualType ResTy;
+  StringLiteral *Stl = nullptr;
+  llvm::APInt Length(32, StrRef.size() + 1);
+  ResTy = S.getASTContext().adjustStringLiteralBaseType(
+      S.getASTContext().WideCharTy.withConst());
+  ResTy = S.getASTContext().getConstantArrayType(ResTy, Length,
+                                                 ArrayType::Normal, 0);
----------------
You already have a suitable string literal as `AL.getArgAsExpr(0)`. There's no need to fabricate another one here.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5471-5474
+  DeclSpecUuidDecl *ArgDecl;
+
+  ArgDecl = DeclSpecUuidDecl::Create(S.getASTContext(), S.getFunctionLevelDeclContext(),
+                                     SourceLocation(), Stl);
----------------
Please combine these lines.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5473-5474
+
+  ArgDecl = DeclSpecUuidDecl::Create(S.getASTContext(), S.getFunctionLevelDeclContext(),
+                                     SourceLocation(), Stl);
+
----------------
You should check for a prior `DeclSpecUuidDecl` with the same UUID here and set it as the previous declaration of the new one, so that they're treated as declaring the same entity.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5476
+
+  S.DeclToDeclSpecUuidDecl[D] = ArgDecl;
+
----------------
(Remove this; with the other change I suggested above this should be unnecessary.)


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

https://reviews.llvm.org/D43576