[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. I was just working on this this afternoon to show you the difference when it is split up. Looks like you committed this meanwhile. While building I noticed that you introduced a `-Wreorder` bug in D57238 , which isn't surprising given

[PATCH] D57104: [AST] Pack GenericSelectionExpr

This revision was automatically updated to reflect the committed changes. Closed by commit rL352276: [AST] Pack GenericSelectionExpr (authored by brunoricci, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D57104?vs=183349=183698#toc

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1371161 , @steveire wrote: > In D57104#1369751 , @riccibruno > wrote: > > > Cleanup the patch by factoring out the NFC changes. > > > > @steveire This should be much clearer

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. In D57104#1369751 , @riccibruno wrote: > Cleanup the patch by factoring out the NFC changes. > > @steveire This should be much clearer now. I'd like to apply this patch locally, but I think the removed parts are not in the

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1370275 , @steveire wrote: > In D57104#1370081 , @riccibruno > wrote: > > > > I highly recommend this 9 minute video if this is new to you or you > > > haven't seen it

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. In D57104#1370081 , @riccibruno wrote: > > I highly recommend this 9 minute video if this is new to you or you haven't > > seen it before: https://youtu.be/qpdYRPL3SVE?t=103 > > I would like to add an additional meta-comment

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. > I highly recommend this 9 minute video if this is new to you or you haven't > seen it before: https://youtu.be/qpdYRPL3SVE?t=103 I would like to add an additional meta-comment here, but please don't take this in a bad way. I am wondering about the usefulness and

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1370025 , @aaron.ballman wrote: > In D57104#1369985 , @steveire wrote: > > > There's definitely a better possible ordering in two commits: > > > > 1. Introduce `::Create` and

[PATCH] D57104: [AST] Pack GenericSelectionExpr

aaron.ballman added a comment. In D57104#1369985 , @steveire wrote: > There's definitely a better possible ordering in two commits: > > 1. Introduce `::Create` and port to it > 2. Use trailing objects, taking advantage of the fact that `::Create` exists.

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1369985 , @steveire wrote: > There's definitely a better possible ordering in two commits: > > 1. Introduce `::Create` and port to it > 2. Use trailing objects, taking advantage of the fact that `::Create` exists. > >

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. In D57104#1369985 , @steveire wrote: > There's definitely a better possible ordering in two commits: > > 1. Introduce `::Create` and port to it > 2. Use trailing objects, taking advantage of the fact that `::Create` exists. > >

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. There's definitely a better possible ordering in two commits: 1. Introduce `::Create` and port to it 2. Use trailing objects, taking advantage of the fact that `::Create` exists. That would make it clear in the future to other people because both commits would be

[PATCH] D57104: [AST] Pack GenericSelectionExpr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D57104#1369885 , @riccibruno wrote: > In D57104#1369864 , @steveire wrote: > > > Thanks, but

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1369864 , @steveire wrote: > Thanks, but I don't see the factored out changes in the repo. Are you going > to commit those first? > > Also, please factor out the introduction and use of `::Create`. It seems to > be

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. Thanks, but I don't see the factored out changes in the repo. Are you going to commit those first? Also, please factor out the introduction and use of `::Create`. It seems to be unrelated to the trailing objects change. Repository: rC Clang CHANGES SINCE LAST

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno updated this revision to Diff 183349. riccibruno marked 6 inline comments as done. riccibruno added a comment. Cleanup the patch by factoring out the NFC changes. @steveire This should be much clearer now. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno marked 3 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5095 - SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; } - SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added inline comments. Comment at: include/clang/AST/Expr.h:5095 - SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; } - SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; } riccibruno wrote: > steveire wrote: > >

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno marked 2 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5095 - SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; } - SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1368292 , @steveire wrote: > In D57104#1368072 , @riccibruno > wrote: > > > In D57104#1368055 , @steveire > > wrote: > > > > >

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. In D57104#1368072 , @riccibruno wrote: > In D57104#1368055 , @steveire wrote: > > > Splitting the introduction of and porting to `Create` would significantly > > reduce the number of

[PATCH] D57104: [AST] Pack GenericSelectionExpr

aaron.ballman added inline comments. Comment at: include/clang/AST/Expr.h:5043 + // association expressions. + enum { CONTROLLING = 0, ASSOC_EXPR_START = 1 }; + Do we want to use `ControllingIndex` and `AssocExprStartIndex` and combine with the enum you

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno marked an inline comment as done. riccibruno added a comment. In D57104#1368055 , @steveire wrote: > Splitting the introduction of and porting to `Create` would significantly > reduce the number of files touched by the 'real' change in this

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. I haven't done a full review as it's not obvious what parts of the diff relate to which separate change. Perhaps Aaron will review and approve though for you anyway. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57104/new/

[PATCH] D57104: [AST] Pack GenericSelectionExpr

steveire added a comment. Splitting the introduction of and porting to `Create` would significantly reduce the number of files touched by the 'real' change in this commit, and therefore reduce noise in the commit (following the idea of "do one thing per commit" to make the code reviewable in

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, steveire. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Store the controlling expression, the association expressions and the corresponding `TypeSourceInfo`s as trailing objects.