[PATCH] D57104: [AST] Pack GenericSelectionExpr

Thu Jan 24 14:55:56 PST 2019

steveire added a comment.

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

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

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

Repository:
  rC Clang

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

https://reviews.llvm.org/D57104