zahiraam updated this revision to Diff 207882.
zahiraam marked 14 inline comments as done.
zahiraam added a comment.
Thanks for the review.
I think and hope that I have responded to every issue you raised. Let me know
if there are still pending issues.
Happy 4th!
CHANGES SINCE LAST ACTION
zahiraam added inline comments.
Comment at: include/clang/AST/Decl.h:4303
+
+ StringLiteral *getSTLUuid() { return STLUuid; }
+};
rsmith wrote:
> What does "STL" mean here?
Renamed it.
Comment at: lib/CodeGen/CodeGenModule.cpp:1071-1073
+
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
zahiraam added a comment.
A review please :-) Thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
zahiraam updated this revision to Diff 200484.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/Attr.td
include/clang/Basic/DeclNodes.td
zahiraam added a comment.
And this patch actually fixes the bug. Thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
zahiraam updated this revision to Diff 200258.
zahiraam marked 2 inline comments as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/Attr.td
zahiraam marked 12 inline comments as done.
zahiraam added a comment.
@rsmith I think I have made all the changes you have pointed out to. But please
note that this new patch only impements an explicit AST representation of uuid
in template arguments. It **does not** fix the bug for which this
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
+
zahiraam added a comment.
There are still a few things I haven't addressed yet. Mostly because not sure
there is another solution like getting rid of the map from StringRef to expr.
The other issue is not adding new kind to ParsedAttr. There may be another way
of doing it, but didn't look at
zahiraam updated this revision to Diff 193258.
zahiraam marked 15 inline comments as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/Attr.td
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
rnk added a comment.
I see! I only took a quick look, but this looks exactly like what @rsmith has
been asking for in discussions for a long time now: a more explicit AST
representation of uuid of uuidof in template arguments. I'll make an effort to
get his attention and see if this addresses
zahiraam updated this revision to Diff 191371.
Herald added a subscriber: jdoerfert.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/Attr.td
zahiraam added a comment.
It would be nice to have a review for this year old (updated) patch. Thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D43576/new/
https://reviews.llvm.org/D43576
___
cfe-commits mailing list
zahiraam updated this revision to Diff 146642.
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/Attr.td
include/clang/Basic/DeclNodes.td
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
zahiraam added a comment.
Richard,
Please let me know if I have answered to all the issues you raised. Thanks.
https://reviews.llvm.org/D43576
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
zahiraam updated this revision to Diff 143321.
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/Basic/Attr.td
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
lib/AST/Decl.cpp
lib/AST/DeclCXX.cpp
lib/CodeGen/CGDecl.cpp
zahiraam marked 3 inline comments as done.
zahiraam added inline comments.
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();
zahiraam marked 4 inline comments as done.
zahiraam added inline comments.
Comment at: lib/Parse/ParseDecl.cpp:572
+DeclSpecUuidDecl *ArgDecl =
+ DeclSpecUuidDecl::Create(Actions.getASTContext(),
+ Actions.getFunctionLevelDeclContext(),
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
zahiraam updated this revision to Diff 142516.
https://reviews.llvm.org/D43576
Files:
include/clang/AST/Decl.h
include/clang/AST/RecursiveASTVisitor.h
include/clang/Basic/Attr.td
include/clang/Basic/DeclNodes.td
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
zahiraam added a comment.
In https://reviews.llvm.org/D43576#1016295, @majnemer wrote:
> We should really, really avoid making this sort of change without first
> trying to desugar uuidof into a reference to a variable. That would solve a
> ton of problems, problems like this one.
Not sure I
rsmith added a comment.
In https://reviews.llvm.org/D43576#1019703, @zahiraam wrote:
> Currently this declaration:
> struct
> __declspec(uuid("{DDB47A6A-0F23-11D5-9109-00E0296B75D3}"))
> S1;
>
> a CXXRecordDecl type is generated with attributes (the uuid being an
> attribute). Are you
zahiraam added a comment.
In https://reviews.llvm.org/D43576#1016588, @rsmith wrote:
> In https://reviews.llvm.org/D43576#1016561, @majnemer wrote:
>
> > Here's my thinking: the `__uuidof` expression literally declares a variable
> > called `_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3` of type
rsmith added a comment.
In https://reviews.llvm.org/D43576#1016561, @majnemer wrote:
> Here's my thinking: the `__uuidof` expression literally declares a variable
> called `_GUID_ddb47a6a_0f23_11d5_9109_00e0296b75d3` of type `__s_GUID` which
> is why it behaves the way it does:
majnemer added a comment.
In https://reviews.llvm.org/D43576#1016512, @rsmith wrote:
> Do we need to also track whether the argument is a pointer or reference to a
> UUID (and also the cv-qualifiers)? For the `Declaration` case, we track this
> by tracking the corresponding parameter type; the
rsmith added a comment.
Do we need to also track whether the argument is a pointer or reference to a
UUID (and also the cv-qualifiers)? For the `Declaration` case, we track this by
tracking the corresponding parameter type; the same thing would presumably work
here.
In
majnemer added a comment.
In https://reviews.llvm.org/D43576#1016418, @zahiraam wrote:
> In https://reviews.llvm.org/D43576#1016295, @majnemer wrote:
>
> > We should really, really avoid making this sort of change without first
> > trying to desugar uuidof into a reference to a variable. That
zahiraam added a comment.
In https://reviews.llvm.org/D43576#1016295, @majnemer wrote:
> We should really, really avoid making this sort of change without first
> trying to desugar uuidof into a reference to a variable. That would solve a
> ton of problems, problems like this one.
Not sure I
zahiraam updated this revision to Diff 135502.
https://reviews.llvm.org/D43576
Files:
test/CodeGenCXX/instantiate-uuid.cpp
test/Sema/member-reference-dll.cpp
Index: test/Sema/member-reference-dll.cpp
===
---
zahiraam added a comment.
Adding test case.
https://reviews.llvm.org/D43576
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
erichkeane added a comment.
It seems to me that the test here is very much lacking. It doesn't seem to
include the example you've mentioned, and has no validation to ensure that
there is a single instantiation happening. I'd like to see what happens when a
UUID is passed as a pack, how
majnemer added a comment.
We should really, really avoid making this sort of change without first trying
to desugar uuidof into a reference to a variable. That would solve a ton of
problems, problems like this one.
https://reviews.llvm.org/D43576
34 matches
Mail list logo
