From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66282 invoked by alias); 1 Dec 2016 19:30:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 53370 invoked by uid 89); 1 Dec 2016 19:30:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=overlapping, ui_out_impl, class-ify, Class-ify X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Dec 2016 19:30:17 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 58452C054912; Thu, 1 Dec 2016 19:30:14 +0000 (UTC) Received: from [127.0.0.1] (ovpn03.gateway.prod.ext.phx2.redhat.com [10.5.9.3]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uB1JUDtC004469; Thu, 1 Dec 2016 14:30:13 -0500 Subject: Re: [PATCH 19/22] Class-ify ui_out_impl To: Simon Marchi References: <20161124152428.24725-1-simon.marchi@polymtl.ca> <1480173587-7053-19-git-send-email-simon.marchi@polymtl.ca> <8ca5a7eb3f6adac16c8db785bd9fa6d3@polymtl.ca> <22e3ae8a5895e196eeaa2393e2995bb6@polymtl.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <5262b594-b95b-75f7-0502-eaecc8987068@redhat.com> Date: Thu, 01 Dec 2016 19:30:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <22e3ae8a5895e196eeaa2393e2995bb6@polymtl.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-12/txt/msg00053.txt.bz2 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. 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