[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

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

> In D57104#1370081 <https://reviews.llvm.org/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 here, but please don't take this in a bad way. I am wondering
> >  about the usefulness and the productivity of the "if this is new to you" in what I am assuming is a discussion
> >  between professionals. I will be happy to address any further technical comments regarding the code itself.
>
>
> Sorry for that. It's a confusing conversation and text doesn't help! :)


I totally agree with that.

> Other classes have `::Create` methods and if it's a justification you're looking for precedent could be it :).
> 
> My proposal is a patch which only does one thing (Pack GenericSelectionExpr) and a commit message that corresponds to that one thing. I'm not proposing 'doing half a thing' in a commit like just inheriting from `llvm::TrailingObjects`.
> 
> Your preference is a patch that packs GenericSelectionExpr and does something else.

I don't think that this is accurate. This patch does one thing: pack `GenericSelectionExpr`. It does this by doing two logically distinct things:
1.) move some data to the bit-fields of `Stmt` and 2) tail-allocate the array of `Stmt *` and the array of `TypeSourceInfo`.

The addition of the `Create` function is part of 2) and not something distinct.

> I don't see how a person in the future would find the former more confusing than the latter. Someone in the future won't care that at the end of January 2019 the thing didn't already have a `::Create` method. There is future-value reducing noise in commits. I know that's fuzzy and there isn't agreement on the specifics, but maybe you agree with the principle. I know many people don't agree with that principle, and many more people never use something like gitk/git log and don't see the value in granular commits (hence why someone went and gave a talk at a conference about it :)).

I agree with you that granular commits (with good and accurate commit messages!) doing one thing are important.
I am (and I think that everyone is) a fan of (git log/blame/etc). It is indeed frustrating to do some git archaeology
and end up to a commit with no explanation.

I believe however that our disagreement here is not on whether we should have good commit messages (duh, of course we should),
nor on whether we should have granular commits (duh, of course we should too), but on whether this is a granular change.

I am arguing that it is and it seems that you are arguing that this is not the case.

> So, I've given that feedback and you have your LGTM. In my book, the rest is up to you! :)

Consensus is important and I can totally miss things so I don't want to just ignore you and push ahead.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104