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