jdenny added a comment.
In https://reviews.llvm.org/D43248#1036439, @jdenny wrote:
> So, I'm planning to remove test/Frontend/ast-attr.cpp, rename
> test/Sema/attr-print.cpp to test/Misc/attr-print-emit.cpp, and change its run
> lines to:
>
> // RUN: %clang_cc1 %s -ast-print | FileCheck %s
>
jdenny added a comment.
In https://reviews.llvm.org/D43248#1036427, @echristo wrote:
> In https://reviews.llvm.org/D43248#1036426, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D43248#1036409, @jdenny wrote:
> >
> > > I'd prefer to move it than to expect people to obey such a comment.
echristo added a comment.
In https://reviews.llvm.org/D43248#1036426, @aaron.ballman wrote:
> In https://reviews.llvm.org/D43248#1036409, @jdenny wrote:
>
> > I'd prefer to move it than to expect people to obey such a comment. Let's
> > see what Aaron says.
>
>
> I have a slight preference for
aaron.ballman added a comment.
In https://reviews.llvm.org/D43248#1036409, @jdenny wrote:
> I'd prefer to move it than to expect people to obey such a comment. Let's
> see what Aaron says.
I have a slight preference for moving the tests now that I know they're causing
problems, unless that t
jdenny added a comment.
I'd prefer to move it than to expect people to obey such a comment. Let's see
what Aaron says.
Repository:
rC Clang
https://reviews.llvm.org/D43248
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llv
echristo added inline comments.
Comment at: test/Sema/attr-print.cpp:3
+// This file is also used as input for %S/../Frontend/ast-attr.cpp.
+
jdenny wrote:
> echristo wrote:
> > Relatedly I don't think we use files as input files to other directories
> > and I
jdenny added inline comments.
Comment at: test/Sema/attr-print.cpp:3
+// This file is also used as input for %S/../Frontend/ast-attr.cpp.
+
echristo wrote:
> Relatedly I don't think we use files as input files to other directories and
> I don't think we should
echristo added inline comments.
Comment at: test/Sema/attr-print.cpp:3
+// This file is also used as input for %S/../Frontend/ast-attr.cpp.
+
Relatedly I don't think we use files as input files to other directories and I
don't think we should really. What are
This revision was automatically updated to reflect the committed changes.
jdenny marked 8 inline comments as done.
Closed by commit rC327405: Reland "[Attr] Fix parameter indexing for
several attributes" (authored by jdenny, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D432
jdenny added a comment.
In https://reviews.llvm.org/D43248#1035509, @aaron.ballman wrote:
> LGTM with the test comments fixed up.
Thanks! I'll commit tomorrow.
https://reviews.llvm.org/D43248
___
cfe-commits mailing list
cfe-commits@lists.llvm.o
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with the test comments fixed up.
Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:5
+// RUN: %clang -emit-ast -o %t.ast %S/../Sema/attr-print.cpp
+// RUN:
jdenny added inline comments.
Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:5
+// RUN: %clang -emit-ast -o %t.ast %S/../Sema/attr-print.cpp
+// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %S/../Sema/attr-print.cpp
aaron.ballman wrote:
> Just to verify my und
aaron.ballman added inline comments.
Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:5
+// RUN: %clang -emit-ast -o %t.ast %S/../Sema/attr-print.cpp
+// RUN: %clang_cc1 %t.ast -ast-print | FileCheck %S/../Sema/attr-print.cpp
Just to verify my understanding, this
jdenny added inline comments.
Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2
+// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse
+// it here to check -emit-ast for attributes.
+
aaron.ballman wrote:
> jdenny wrote:
> > aaron.ballm
aaron.ballman added a comment.
In https://reviews.llvm.org/D43248#1035477, @jdenny wrote:
> In https://reviews.llvm.org/D43248#1035466, @aaron.ballman wrote:
>
> > It seems like there are some other changes than just the serialize and
> > deserialize that I'm not opposed to, but am wondering why
jdenny added inline comments.
Comment at: cfe/trunk/test/Frontend/ast-attr.cpp:1-2
+// %S/../Sema/attr-print.cpp exercises many different attributes, so we reuse
+// it here to check -emit-ast for attributes.
+
aaron.ballman wrote:
> Can you move this below the R
jdenny added a comment.
In https://reviews.llvm.org/D43248#1035466, @aaron.ballman wrote:
> It seems like there are some other changes than just the serialize and
> deserialize that I'm not opposed to, but am wondering why they're needed. It
> seems some functions are now `getFoo()` calls
The
aaron.ballman added a comment.
It seems like there are some other changes than just the serialize and
deserialize that I'm not opposed to, but am wondering why they're needed. It
seems some functions are now `getFoo()` calls and it seems like some
declarations moved around. Are those intended a
jdenny updated this revision to Diff 138113.
jdenny added a comment.
Well, that didn't work. Here's another attempt at getting the paths right.
https://reviews.llvm.org/D43248
Files:
cfe/trunk/include/clang/AST/Attr.h
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/lib/AST/ExprConstant.c
jdenny updated this revision to Diff 138111.
jdenny added a comment.
OK, this diff has the svn paths, and I've rebased to a more recent master.
https://reviews.llvm.org/D43248
Files:
trunk/include/clang/AST/Attr.h
trunk/include/clang/Basic/Attr.td
trunk/lib/AST/ExprConstant.cpp
trunk/li
aaron.ballman added a comment.
Can you regenerate the patch using the same paths as
https://reviews.llvm.org/D43248?id=136811? When I try to do a diff between what
was accepted & committed and the current patch, Phabricator gets confused
because the paths are too different from one another.
h
jdenny updated this revision to Diff 137964.
jdenny added a comment.
This commit was reverted by r326862 due to:
https://bugs.llvm.org/show_bug.cgi?id=36620
This revision includes a new test case and a fix.
While the difference from the last revision is small, it's not trivial, so
another revi
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326602: [Attr] Fix parameter indexing for several attributes
(authored by jdenny, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43248?vs=136
aaron.ballman added a comment.
In https://reviews.llvm.org/D43248#1025495, @jdenny wrote:
> Aaron, thanks for the review. I've applied your suggestions and am ready to
> commit.
You're correct that we have a lot of freedom with the commit message, but the
important piece is in describing wha
jdenny added a comment.
Aaron, thanks for the review. I've applied your suggestions and am ready to
commit.
I've noticed a variety of styles in commit logs, I've read the coding
standards, and it seems there's a lot of freedom here. Below are the two
commit logs I'm planning to use. Please
aaron.ballman added inline comments.
Comment at: test/Sema/attr-ownership.cpp:1
+// RUN: %clang_cc1 %s -verify
+
jdenny wrote:
> aaron.ballman wrote:
> > Please pass `-fsyntax-only` as well.
> Sure. Would you like me to change test/Sema/attr-ownership.c while we
jdenny added inline comments.
Comment at: test/Sema/attr-ownership.cpp:1
+// RUN: %clang_cc1 %s -verify
+
aaron.ballman wrote:
> Please pass `-fsyntax-only` as well.
Sure. Would you like me to change test/Sema/attr-ownership.c while we're
thinking about it?
h
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Aside from two minor nits, this LGTM. Thank you for working on it!
Comment at: test/Sema/attr-ownership.cpp:1
+// RUN: %clang_cc1 %s -verify
+
jdenny added inline comments.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx &I) const {
+assert(isValid() && I.isValid() &&
aaron.ballman wrote:
> jdenny wrote:
> > aaron.ballman wrote:
> > > The name here can be improved. How abou
jdenny updated this revision to Diff 136624.
jdenny marked 23 inline comments as done.
jdenny edited the summary of this revision.
jdenny added a comment.
This update should address all outstanding comments.
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic
jdenny marked 3 inline comments as done.
jdenny added inline comments.
Comment at: include/clang/Basic/Attr.td:172-174
+ // Whether the C++ implicit this parameter is allowed. Users that construct
+ // attributes from the source code use this information when validating
+ //
aaron.ballman added inline comments.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx &I) const {
+assert(isValid() && I.isValid() &&
jdenny wrote:
> aaron.ballman wrote:
> > The name here can be improved. How about `checkInvariants()
jdenny added a comment.
In https://reviews.llvm.org/D43248#1024183, @aaron.ballman wrote:
> In https://reviews.llvm.org/D43248#1023720, @jdenny wrote:
>
> > Hi Aaron. It occurs to me now that this patch has grown rather large and,
> > in some places, a little subtle. Would it help the review i
aaron.ballman added a comment.
In https://reviews.llvm.org/D43248#1023720, @jdenny wrote:
> Hi Aaron. It occurs to me now that this patch has grown rather large and, in
> some places, a little subtle. Would it help the review if I were to break it
> up into a patch series that introduces Para
aaron.ballman added inline comments.
Comment at: include/clang/AST/Attr.h:206
+
+ void cmpable(const ParamIdx &I) const {
+assert(isValid() && I.isValid() &&
The name here can be improved. How about `checkInvariants()`? Might as well
make this inline while
jdenny added a comment.
Hi Aaron. It occurs to me now that this patch has grown rather large and, in
some places, a little subtle. Would it help the review if I were to break it
up into a patch series that introduces ParamIdx to each attribute, one at a
time? I'm not trying to rush you, but
jdenny added inline comments.
Comment at: include/clang/AST/Attr.h:210-212
+ unsigned Idx;
+ bool HasThis;
+ bool IsValid;
aaron.ballman wrote:
> I think it might be best to mash these together using bit-fields:
> ```
> unsigned Idx : 30;
> unsigned HasThis :
jdenny updated this revision to Diff 136019.
jdenny marked 8 inline comments as done.
jdenny edited the summary of this revision.
jdenny added a comment.
This revision should address all issues raised.
https://reviews.llvm.org/D43248
Files:
include/clang/AST/Attr.h
include/clang/Basic/Attr.
aaron.ballman added a comment.
(Review is incomplete, but here are some initial comments.)
Comment at: include/clang/AST/Attr.h:210-212
+ unsigned Idx;
+ bool HasThis;
+ bool IsValid;
I think it might be best to mash these together using bit-fields:
```
unsi
jdenny updated this revision to Diff 135839.
jdenny retitled this revision from "[Attr] Fix printing of parameter indices in
attributes" to "[Attr] Fix parameter indexing for attributes".
jdenny edited the summary of this revision.
jdenny added a comment.
Herald added a subscriber: kristof.beyls.
40 matches
Mail list logo
