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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
