Re: [RFA v2 16/17] Convert dwarf_expr_context_funcs to methods

[Pedro Alves <palves at redhat dot com>]

On 10/18/2016 03:03 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> All I saw was the comment about possibly reducing churn:
>>> https://sourceware.org/ml/gdb-patches/2016-10/msg00208.html
> 
> Pedro> I'm sorry, I had written comments about the parenthesis
> Pedro> thing and also about wondering whether these methods with
> Pedro> internal_error could be pure virtual ('= 0;') instead, and if not,
> Pedro> about adding a small comment mentioning it.
> 
> I think the internal_error thing can't be a pure virtual because there
> are subclasses that do not implement this method.  I could push the
> internal error into the subclasses though.

Hmm.  I guess the actual code smell is that these are internal_errors
instead of normal errors in the first place?  I.e., why are these two
cases internal_errors when the rest of the virtual methods have
defaults that call normal error instead?

Put another way, is there something in the inheritance design that
makes calling these default implementations a gdb bug?   Cases like
"virtual foo() and virtual bar() work in tandem, so if you get to
the default bar(), it means you overrode foo() but forgot bar()" would
be those which I think would be reasonable to internal_error.  I.e.,
clear implementation bugs.

So I'm wondering whether invalid dwarf (i.e., input) or something
like that could trigger these internal errors?

In any case, I see now that the old code internal_errors as well,
so in the name of keeping the patch's scope be converting function
pointer tables to virtual methods, it makes sense to keep the
internal_errors.

So patch is fine as is with the internal_error.  Thanks for the patience.

Thanks,
Pedro Alves