[PATCH 000/203] Refactor expressions

[Joel Brobecker]

Hi Tom,

On Fri, Jan 01, 2021 at 02:44:00PM -0700, Tom Tromey wrote:
> I have long thought that struct expression is the worst data structure
> in GDB.  In fact, I've sometimes told people it is the worst data
> structure I've encountered in my career.  I've wanted to rewrite it
> for many years, and this year I finally found a workable plan and the
> motivation to do so.  This series is the result.

Thanks for the (mega) series!

Because of the length of the series, I think it's going to be potentially
more difficult for you to maintain it over time. I so I think it would
be nice if we could put a priority on its review.

I've now started with part 1:

> 1. Split functions out from the evaluators.
> 
>    Each evaluator function is fairly long, and mixes knowledge about
>    the structure of the expression (how the exp_elements are laid out)
>    with knowledge of the semantics of the operation.
> 
>    This part of the series, which is about 70 patches, splits out the
>    semantics of most operations into their own functions.  These
>    functions can be then be reused -- which is helpful for ensuring
>    that changes don't sneak in.
> 
>    In this part of the series, sometimes the newly-introduced
>    functions have extraneous arguments.  This was done partly so that
>    some could be reused by generic code in part 2; but also partly
>    because it was just simpler to write patches by rote.

Aside from the commit "what is this code for", I think this part
of the series is a nice improvement on its own. independently
of redesigning struct expression, I've always found the evaluator
functions to be overly long and harder to navigate as a result.
So I think it could go in ahead of the rest if we agree that
this part is good.

For me, I've gone through the patches, more or less carefully
based on a random sample, and they look good. I paused a bit
about the Ada ones, were you excluded the hanlding of noside ==
EVAL_SKIP. I'm not entirely sure why that is, perhaps because
the block consists in a goto nosideret? Looking at what that
nosideret does, it's just...

    | nosideret:
    |   return eval_skip_value (exp);

... so we could inline this in the new functions.

However, this is really a very minor detail that doesn't need to be
addressed here.

So, to summarize, if others agree, I'm happy for this part of
the series to go in.

> 
> 2. Introduce 'class operation' and its subclasses.
> 
>    This sub-series is around 100 patches.  It introduces the new base
>    class for an expression operation, and then proceeds to add
>    operations for every necessary opcode.  In some cases multiple such
>    operations are needed -- for example when multiple languages
>    implement an opcode in different ways.
> 
>    Some discussion of the design & tradeoffs appear in the "Introduce
>    class operation" patch.
> 
> 3. Convert existing code to use operations.
> 
>    This is a short sub-series of around 10 patches.  Each parser is
>    converted separately, as are DTrace and SystemTap probes
> 
> 4. Delete obsolete code.
> 
>    The final 20 patches or so are concerned with removing all the code
>    that is now obsolete, and with doing some minor tidying of the
>    result.
> 
> The overall approach here is to replace the current data structure --
> but only the data structure.  I didn't generally try to clean up
> things that seemed a bit weird.
> 
> I also didn't try to add new features, like async evaluation.  After
> starting the project, I felt that trying to combine this refactoring
> with another, different one would be too difficult.
> 
> This is all on the branch t/expression-rewrite-incr in my github.
> 
> Regression tested on x86-64 Fedora 32.  I also built it on PPC Linux
> to ensure that the ppc-linux-nat.c changes would build.
> 
> Tom
> 

-- 
Joel