Re: [PATCH 073/203] Introduce class operation

Joel Brobecker
2021-01-03T07:09:21Z

Hi Tom,

> This patch introduces class operation, the new base class for all
> expression operations.
>
> In the new approach, an operation is simply a class that presents a
> certain interface. Operations own their operands, and management is
> done via unique_ptr.
>
> The operation interface is largely ad hoc, based on the evolution of
> expression handling in GDB. Parts (for example,
> evaluate_with_coercion) are probably redundant; however I took this
> approach to try to avoid mixing different kinds of refactorings.
>
> In some specific situations, rather than add a generic method across
> the entire operation class hierarchy, I chose instead to use
> dynamic_cast and specialized methods on certain concrete subclasses.
> This will appear in some subsequent patches.
>
> One goal of this work is to avoid the kinds of easy-to-make errors
> that affected the old implementation. To this end, some helper
> subclasses are also added here. These helpers automate the
> implementation of the 'dump', 'uses_objfile', and 'constant_p'
> methods. Nearly every concrete operation that is subsequently added
> will use these facilities. (Note that the 'dump' implementation is
> only outlined here, the body appears in the next patch.)
>
> gdb/ChangeLog
> 2021-01-01 Tom Tromey <tom@tromey.com>
>
> * expression.h (expr::operation): New class.
> (expr::make_operation): New function.
> (expr::operation_up): New typedef.
> * expop.h: New file.
> * eval.c (operation::evaluate_for_cast)
> (operation::evaluate_for_address, operation::evaluate_for_sizeof):
> New methods.
> * ax-gdb.c (operation::generate_ax): New method.

Thanks for this!

I don't have much comment to share on this, at least not just yet
until I have had a chance to see this new API in action in the rest
of the patch series. I do have one small-tiny-mini suggestion:

It's about:

> +typedef std::unique_ptr<operation> operation_up;

FWIW, the connection between "up" and the rest of the declaration
wasn't immediately obvious to me. I think this is because "up"
is an english word, and spelled like that, all in lowercase,
that's really what my brain keeps analyzing it as first. It takes
an effort to adjust my thinking against this bias.

It's not a problem for me if we decide to keep this like that,
but what do you think about about renaming this to something like
"operation_uptr"? I think it'll make the intention more immediately
clear to the reader.

--
Joel