[PATCH 000/203] Refactor expressions

[Andrew Burgess]

* Joel Brobecker <brobecker@adacore.com> [2021-01-03 11:02:50 +0400]:

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

I agree with this.  I looked through all the patches 001 -> 072.
There's a few missing comments that I think should be added, and I
would suggest dropping the "what is this code for" patch (if this
needs looking at it can be a separate branch), but otherwise this
first set would be a good improvement on its own and worth merging
sooner rather than later I think.

Thanks,
Andrew



> 
> > 
> > 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