[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D57104#1368072 <https://reviews.llvm.org/D57104#1368072>, @riccibruno wrote:

> In D57104#1368055 <https://reviews.llvm.org/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 commit, and therefore reduce noise in the commit (following the idea of "do one thing per commit" to make the code reviewable in the future).
> >
> > However, if you're opposed to that, it's not a hard requirement.
>
>
> To be honest I don't really see the point.


Yes, the `Create` refactor a simple change. The point is to remove the noise of the simple change from the complex change so that the complex change becomes visible.

It would help reviewers like myself who are less familiar with what refactoring for tail allocation involves. I couldn't read your commit and know how to do it for another class because this commit is full of noise.

Anyway, if making the code reviewable (also in the future!) like that is not important enough, I won't block this one! :)



================
Comment at: include/clang/AST/Expr.h:5095
 
-  SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }
----------------
Why remove the `LLVM_READONLY` from this class instead of from everywhere in the file it is not needed? (in a separate commit, obviously).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57104/new/

https://reviews.llvm.org/D57104