[PATCH] D57104: [AST] Pack GenericSelectionExpr

Wed Jan 23 12:50:50 PST 2019

riccibruno added a comment.

In D57104#1368292 <https://reviews.llvm.org/D57104#1368292>, @steveire wrote:

> 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! :)

You are right that readability and review-ability is pretty important. My apologies for making this an unreadable mess :)
Let me clean this patch a little bit (along with addressing Aaron's comments).

Repository:
  rC Clang

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

https://reviews.llvm.org/D57104