rsmith added inline comments.
================
Comment at: include/clang/AST/Decl.h:4281
+ class DeclSpecUuidDecl : public Decl {
+ StringRef StrUuid;
+ public:
----------------
Can we store a `StringLiteral*` here instead?
================
Comment at: include/clang/Sema/Sema.h:393-394
+ /// List of declspec(uuid ...) for a specific uuid string.
+ SmallVector<Decl *, 1> DeclSpecUuidDecls;
+
----------------
rsmith wrote:
> Remove this? It is only used once, and only to initialize an unused variable.
Comment marked done but not done?
================
Comment at: include/clang/Sema/Sema.h:399
+ std::map<StringRef, ExprResult> UuidExpMap;
+ std::map<const Decl*, StringRef> DeclSpecToStrUuid;
+
----------------
rsmith wrote:
> This seems suspicious. If we want the uuid of a declaration, should we not be
> asking it for its `UuidAttr` instead?
Comment marked done but not done?
================
Comment at: lib/Parse/ParseDecl.cpp:572
+ DeclSpecUuidDecl *ArgDecl =
+ DeclSpecUuidDecl::Create(Actions.getASTContext(),
+ Actions.getFunctionLevelDeclContext(),
----------------
zahiraam wrote:
> rsmith wrote:
> > 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.
> If we want to align with MS, we don't want to signal an error. So may be I
> should have a map that assigns to each StrUuid a list of DeclSpecUuid?
> So for this code:
> struct
> __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
> S1 {};
>
> struct
> __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
> S2 {};
>
> I will have a map from DDB47A6A-0F23-11D5-9109-00E0296B75D3 to {S1, S2}. Do
> you agree?
>
I think you should have a map from UUID to either the most recent
`DeclSpecUuidDecl` for that UUID, and form redeclaration chains for
`DeclSpecUuidDecl`s with the same UUID. (`DeclSpecUuidDecl` should derive from
`Redeclarable<DeclSpecUuidDecl>` and you can call `setPreviousDecl` to connect
the new declaration to the prior one.)
You'll need to make sure this case is properly handled during lazy AST
deserialization.
================
Comment at: lib/Parse/ParseDecl.cpp:594
+ bool Invalid = false;
+ StringRef UuidStr = PP.getSpelling(Tok, UuidBuffer, &Invalid);
+
----------------
zahiraam wrote:
> rsmith wrote:
> > How does this handle the case where multiple tokens must be concatenated to
> > form a uuid? Eg, `uuid(12345678-9abc-def0-1234-56789abcdef0)`
> ksh-3.2$ cat m3.cpp
> struct __declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;
> ksh-3.2$ clang -c m3.cpp
> m3.cpp:1:35: error: invalid digit 'a' in decimal constant
> struct __declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;
> ^
> m3.cpp:1:39: error: use of undeclared identifier 'def0'
> struct __declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;
> ^
> m3.cpp:1:54: error: invalid digit 'a' in decimal constant
> struct __declspec (uuid(12345678-9abc-def0-1234-56789abcdef0)) s;
> ^
> 3 errors generated.
> ksh-3.2$
>
This case is accepted by MSVC and not here:
```
struct __declspec(uuid(R"(12345678-9abc-def0-1234-56789abcdef0)")) s;
```
(MSVC also accepts a `u8` prefix, but no other encoding prefix. I also note
that MSVC does not support string literal concatenation here, so we don't need
to deal with that.)
Please use `StringLiteralParser` to properly handle cases like this, rather
than rolling your own string literal parsing here.
================
Comment at: lib/Sema/SemaExprCXX.cpp:619
+ /// Finds the __declSpec uuid Decl off a type.
+ static void FindADeclOffAType(Sema &SemaRef,
+ QualType QT,
----------------
Do we need both this and getUuidAttrOfType?
================
Comment at: lib/Sema/SemaExprCXX.cpp:635
+ if (TD->getMostRecentDecl()->getAttr<UuidAttr>()) {
+ UuidDecl.push_back(dcl);
+ return;
----------------
It would seem cleaner to collect the `UuidAttrDecl`s or `UuidAttr`s here rather
than the declarations that have those attributes.
================
Comment at: lib/Sema/SemaExprCXX.cpp:647-648
+ FindADeclOffAType(SemaRef, TA.getAsType(), UuidDecl);
+ if (dd)
+ UuidDecl.push_back(dcl);
+ }
----------------
Dead code.
================
Comment at: lib/Sema/SemaExprCXX.cpp:675-681
+ if (expr.isUnset()) {
+ uuid_expr =
+ new (Context) CXXUuidofExpr(TypeInfoType.withConst(), Operand, UuidStr,
+ SourceRange(TypeidLoc, RParenLoc));
+ UuidExpMap[UuidStr] = uuid_expr;
+ return uuid_expr;
+ }
----------------
Revert this change; we need to build a new `CXXUuidofExpr` with a correct
source location and type rather than reusing one from somewhere else. Then
remove `UuidExpMap`.
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
