[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?

[David Greene via llvm-dev]

Joerg Sonnenberger via cfe-dev <cfe-dev at lists.llvm.org> writes:

> Let's try to be a bit less theoretical. Consider that we want to
> introduce a new optimisation. The patch set contains three parts:
> (1) Refactoring of some existing code in preparation of reusing it.
> (2) Adding test cases for existing cases that should be preserved as is.
> (3) The new optimisation with matching tests.
>
> Change (1) and (2) are independent of each other, but neither makes much
> sense when (3) is rejected. As such, creating a patch series for review
> is more sensible than submitting independent changes. Now when there is
> agreement that (3) is desirable, it is not so unusual to see some back
> and forth still going on for (1). In that case, a LGTM for (2) allows
> taking that part of out of the patch series, allowing (new) reviewers to
> focus on the parts still under active consideration.

Thanks for these examples, they are helpful!

I'm not sure (1) and (2) are useless if (3) is rejected.  Making code
more reusable is often desirable in its own right.  Adding more tests is
often desirable in its own right.  If nothing else, working on (3) has
brought to light some issues that hadn't been considered before (holes
in testing, suboptimal code design).  It seems to be that committing
those changes independent of (3) is not necessarily wrong.

But your larger point stands, there may in general be cases where (1)
and (2) are not desired without (3).

> Again, let's try to pick a realistic example. Constant folding of fast
> math for example. That includes a lot of very similar cases in multiple
> places. There will be a lot of similarity in the code and there is a
> common topic, but sub-topics like operator classes tend to be independet.
> As such, it is helpful for the submitter and the reviewer to group them
> together as patch series. Whenever something is done, it can drop out of
> the review.

I can see the reason for linking them in a series, but is it really the
case that we would want some of these changes committed and not others?
That's essentially what we're saying by making them separate patches.  I
would tend to put them all in one patch.

But again, in general I think your example certainly can apply to some
situations.

For both cases, I am not sure how often they apply or if there is a
different way of thinking about them that doesn't involve a patch series
(for example doing one patch for example 2).

                     -David