Re: [PATCH 19/22] Class-ify ui_out_impl

[Pedro Alves <palves@redhat.com>]

On 12/01/2016 07:04 PM, Simon Marchi wrote:
> On 2016-11-30 17:57, Pedro Alves wrote:
>> On 11/30/2016 10:38 PM, Simon Marchi wrote:
>>
>>> I'll consider working on merging ui_out and ui_out_impl_* in a single
>>> class hierarchy.  My first question is: what is a good pattern for the
>>> overlapping methods.  For example, table_begin.  We'll want to execute
>>> the version of the base class, which will then call the specialization.
>>> So we can't use the same name for both.  So, keep table_begin for the
>>> base class, and table_begin_impl for the derived classes?
>>
>> Yes.  I think gold's convention is to use $method for public methods, and
>> do_$method for protected virtual methods that implementations
>> override/provide.  I've seen "do_" used for the same purpose in other
>> projects too.  (IIRC, gold is even stricter and requires that virtual
>> methods must be protected, thus mandating that design everywhere.
>> Grep for "this->do_".)
> 
> As I am doing this, I realize we'll need a cast anyway, it just changes
> where it is.
> 
> For example, mi_cmd_var_list_children calls "mi_version (uiout)".  The
> uiout variable is of type "ui_out *", since we don't know at compile
> time that the current_uiout will be an MI uiout.  We know it will be
> because of how the program works, but that can only be verified at
> runtime.  So in the end, instead of doing a dynamic_cast in mi_version,
> we'll have to do one in the function using the uiout.

In that case, I'd say that problem is that that code uses "current_uiout".
If we walk up the call stack, we should find the MI interpreter somewhere,
which has a handle to the MI uiout, should know its type.  If somehow one
of those was passed down, either by parameter (or by being able to access
them from some existing MI-specific global context structure, maybe the
existing "current_context"), then the problem wouldn't exist.  We'd still be
shifting the casts higher up a bit, as we still have things like
as_mi_interp, which are effectively dynamic_casts in the ui<->interp code.
That one would probably be sorted by cleaning up the "virtual" methods
of "struct interpreter" and "struct ui", e.g., by adding an
"interpreter->execute_command (....)" virtual method.  Certainly
nothing urgent to fix, but I think good to keep in mind.

So as long as we're shifting casts closer to the top level, it seems like
we're heading in the proper direction.

BTW, in code that knows what the dynamic type of a pointer should be, we
should feel free to use static_cast<derived *>.  But I'm fine with
using dynamic_cast<> as validation aid.  Just be aware that it's not
free performance-wise.

> I still think that merging ui_out and ui_out_impl is a good idea though,
> there's no good reason for them to be separate.

*nod*

Thanks,
Pedro Alves