rsmith added a comment.
Thanks, this looks like a good broad direction.
I think this patch does not goes far enough yet. Broad comments (partially
duplicating some of the specific comments on the patch itself):
- form the `DeclSpecUuidDecl` earlier, when parsing the attribute argument
- store a pointer to the `DeclSpecUuidDecl` on a non-dependent `CXXUuidofExpr`
- remove the maps in Sema, and instead ask attributes and `uuidof` expressions
for their declarations
- `DeclSpecUuidDecl` creation needs to either deduplicate declarations with the
same UUID or form redeclaration chains for such declarations
- we are missing at least serialization and deserialization code for the new
declaration kind (and likely other things too: ASTImporter support, for
instance)
- we should be able to emit `DeclSpecUuidDecl`s in CodeGen, replacing our
current ad-hoc emission of UUID globals
================
Comment at: include/clang/Sema/ParsedAttr.h:220
+ StringRef StrUuid;
+
----------------
I don't really like adding new special kinds of argument representation in
`ParsedAttr`, but... this really is a very special kind of argument, with rules
different from anything else.
Have you considered forming the `DeclSpecUuidDecl` object earlier (when the
argument is parsed) and storing it here? That'd be a closer match for how we
handle other forms of attribute arguments (particularly expressions).
================
Comment at: include/clang/Sema/ParsedAttr.h:564
+ StringRef getUuidStr() const {
+ assert(getKind() == AT_Uuid && "Not an availability attribute");
+ return StrUuid;
----------------
Copy-paste error in assert message
================
Comment at: include/clang/Sema/Sema.h:393-394
+ /// List of declspec(uuid ...) for a specific uuid string.
+ SmallVector<Decl *, 1> DeclSpecUuidDecls;
+
----------------
Remove this? It is only used once, and only to initialize an unused variable.
================
Comment at: include/clang/Sema/Sema.h:398
+ /// a single Expr.
+ std::map<StringRef, ExprResult> UuidExpMap;
+ std::map<const Decl*, StringRef> DeclSpecToStrUuid;
----------------
Use an `llvm::StringMap` here? But I'm also not convinced this should exist at
all.
================
Comment at: include/clang/Sema/Sema.h:399
+ std::map<StringRef, ExprResult> UuidExpMap;
+ std::map<const Decl*, StringRef> DeclSpecToStrUuid;
+
----------------
This seems suspicious. If we want the uuid of a declaration, should we not be
asking it for its `UuidAttr` instead?
================
Comment at: lib/Parse/ParseDecl.cpp:588
return !HasInvalidAccessor;
+ if (AttrName->getName() == "uuid") {
+ // Parse the uuid attribute and create a UuidDecl.
----------------
This code is dead: note that we returned on the line before.
================
Comment at: lib/Parse/ParseDecl.cpp:589
+ if (AttrName->getName() == "uuid") {
+ // Parse the uuid attribute and create a UuidDecl.
+ ConsumeParen();
----------------
This comment does not describe what this code does.
================
Comment at: lib/Parse/ParseDecl.cpp:594
+ bool Invalid = false;
+ StringRef UuidStr = PP.getSpelling(Tok, UuidBuffer, &Invalid);
+
----------------
How does this handle the case where multiple tokens must be concatenated to
form a uuid? Eg, `uuid(12345678-9abc-def0-1234-56789abcdef0)`
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5371
+ if (auto *UA = D->getAttr<UuidAttr>()) {
+ if (UA->UuidAttr::getUuidAsStr().equals_lower(Uuid->getStrUuid()))
return nullptr;
----------------
We should be comparing to see if the declarations are the same here (use
`declaresSameEntity` on the UUID decls).
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5404-5409
+ // GUID string shouldn't be a wide string.
+ if (StrRef.front() == 'L') {
+ S.Diag(LiteralLoc, diag::err_attribute_argument_type)
+ << AL.getName() << AANT_ArgumentString;
+ return;
+ }
----------------
Sema should not be dealing with lexical issues like this. This should have been
handled when parsing the attribute argument. (Also this is missing handling for
other encoding prefixes and suffixes.)
================
Comment at: lib/Sema/SemaExprCXX.cpp:636
+ if (TD->getMostRecentDecl()->getAttr<UuidAttr>()) {
+ const Decl *dl = SemaRef.DeclSpecUuidDecls.back();
+ UuidDecl.push_back(dcl);
----------------
This variable is unused. (And it's not clear why we would assume that the
most-recently-created `DeclSpecUuidDecl` would correspond to anything relevant
here.)
================
Comment at: lib/Sema/SemaExprCXX.cpp:664-670
+ FindStrUuid(*this, QT, UuidDecl);
+ if (UuidDecl.empty())
return ExprError(Diag(TypeidLoc, diag::err_uuidof_without_guid));
- if (UuidAttrs.size() > 1)
+ if (UuidDecl.size() > 1)
return ExprError(Diag(TypeidLoc, diag::err_uuidof_with_multiple_guids));
- UuidStr = UuidAttrs.back()->getGuid();
+ const Decl *dd1 = UuidDecl.back();
+ UuidStr = DeclSpecToStrUuid[dd1];
----------------
I think this should be finding the `UuidAttr` of the declaration and asking it
for its `DeclSpecUuidDecl`.
================
Comment at: lib/Sema/SemaExprCXX.cpp:675-678
+ if (expr.isUnset()) {
+ uuid_expr =
+ new (Context) CXXUuidofExpr(TypeInfoType.withConst(), Operand, UuidStr,
+ SourceRange(TypeidLoc, RParenLoc));
----------------
We should store a pointer to the UUID declaration on a non-dependent
`CXXUuidofExpr`.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:554-558
+ DeclSpecUuidDecl *Inst = DeclSpecUuidDecl::Create(SemaRef.Context, Owner,
+ D->getLocation(),
+ D->getStrUuid());
+ Owner->addDecl(Inst);
+ return Inst;
----------------
Just use `llvm_unreachable` here; `DeclSpecUuidDecl`s are global and shouldn't
be instantiated.
================
Comment at: test/Parser/MicrosoftExtensions.cpp:79
- __uuidof(struct_with_uuid);
+ __uuidof(struct_with_uuid); // expected-error {{non-type template argument
of type 'const _GUID' is not a constant expression}}
__uuidof(struct_with_uuid2);
----------------
Why is this error now appearing on the wrong line?
================
Comment at: test/Parser/ms-square-bracket-attributes.mm:22-23
[uuid(u8"000000A0-0000-0000-C000-000000000049")] struct struct_with_uuid_u8;
-// expected-error@+1 {{uuid attribute contains a malformed GUID}}
+// expected-error@+1 {{attribute requires a string}}
[uuid(L"000000A0-0000-0000-C000-000000000049")] struct struct_with_uuid_L;
----------------
This is a diagnostic quality regression. The attribute does not require a
string. But it does not accept a string with an encoding prefix.
================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:332
+ if (type == "FunctionDecl *" || type == "NamedDecl *" ||
+ (type == "DeclSpecUuidDecl *")) {
OS << " OS << \" \";\n";
----------------
Nit: no need for parens here, and bad indentation.
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
