> On Mon, 17 Sep 2018 at 11:20, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> I am fine with that. It was never meant to be committed. I used to run those
> tests to make sure that any changes to the core logic do not break any
> working scenarios. Whenever I found a new failure in the extra tests which
> wasn't there in tests to be committed, I used to move that test from the
> first to the second. Over the time, the number of new failures in extra has
> reduced and recently I didn't see any extra failures. So, may be it's time
> for the extra tests to be dropped. I will suggest that keep the extra tests
> running from time to time and certainly around the time the feature gets
> committed.
Great, that's exactly what I'm doing right now - I keep these tests locally
to monitor any significant failures except any changes in plans, but without
including it into the patch series.
Since most of my complaints about the patch were related to code readability,
I modified it a bit to show more clearly what I have in mind. I haven't changed
anything about the functionality, just adjusted some parts of it:
* removed some unused arguments (looks like they were added for consistency in
all higher level branches, but not all of them were actually in use)
* replaced representation for partition mapping (instead of int arrays there is
a structure, that allows to replace 0/1 with more meaningful from/to)
* tried to make naming a bit more consistent - so, if a function doesn't
explicitely say anything about outer/inner, we have partmap1/partmap2,
otherwise outermap/innermap. I don't really like this style with
partmap1/partmap2 or index1/index2, but it seems consistent with the rest of
the code in partbounds.c
* removed some state representation flags, e.g. merged - instead, if there is a
situation when we can't merge, functions will return NULL right away
* removed questionable debugging statement
Ashutosh, can you please take a look at it and confirm (or not) that you also
think these changes have improved readability a bit. If we're on the same page
about that, I'll continue in this direction.