From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119568 invoked by alias); 18 Oct 2016 20:30:47 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 119488 invoked by uid 89); 18 Oct 2016 20:30:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.5 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=UD:read-md.c, readmdc, rtx_reader, read-md.c 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; Tue, 18 Oct 2016 20:30:36 +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 08D5D8553F; Tue, 18 Oct 2016 20:30:35 +0000 (UTC) Received: from vpn-236-220.phx2.redhat.com (vpn-236-220.phx2.redhat.com [10.3.236.220]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9IKUYdF003912; Tue, 18 Oct 2016 16:30:34 -0400 Message-ID: <1476822633.10766.69.camel@redhat.com> Subject: Re: [PATCH 09/16] Split class rtx_reader into base_rtx_reader vs rtx_reader From: David Malcolm To: Bernd Schmidt , gcc-patches@gcc.gnu.org Cc: Richard Sandiford Date: Tue, 18 Oct 2016 20:30:00 -0000 In-Reply-To: <2296dd4b-cf1e-8fca-2df3-092aad7dc575@redhat.com> References: <1475684110-2521-1-git-send-email-dmalcolm@redhat.com> <1475684110-2521-10-git-send-email-dmalcolm@redhat.com> <2296dd4b-cf1e-8fca-2df3-092aad7dc575@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg01481.txt.bz2 [CCing Richard; this is re: https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00273.html ] Essentially I want to split class rtx_reader into two parts: a base class covering the things implemented in read-md.o, and a subclass implemented in read-rtl.o. The motivation is that I want to make some of the rtx-reading functions in read-rtl.c virtual in a followup, so that they can be overridden by the function_reader subclass in my RTL frontend. Hence the ctor needs access to these functions in order to access the vtable - or the link fails. It appears that I can't simply use read-rtl.o everywhere due to read-rtl.c needing insn-constants.h, which is built by genconstants, which would be a circular dependency, hence the split between BUILD_MD vs BUILD_RTL in Makefile.in to break this cycle. On Tue, 2016-10-11 at 17:53 +0200, Bernd Schmidt wrote: > On 10/05/2016 06:15 PM, David Malcolm wrote: > > - rtx_reader_ptr->get_top_level_filename ()); > > + base_rtx_reader_ptr->get_top_level_filename ()); > > I wonder if the number of changes could be minimized by retaining the > name rtx_reader for the base class, and using something more specific > for the derived ones. Or would that require renaming a bunch of other > locations? The number of changes got rather larger with r241293 ("[PATCH] read-md.c: Move various state to within class rtx_reader (v3)"; https://gcc.gnu.org/ml/gcc-patches/2016-10/msg01323.html ), so I'd rather just "bite the bullet" and do the renamings. I'm not in love with the names I chose in this patch. It does seem odd having an "rtx_reader" class that can't actually read hierarchical rtx. How about "md_reader" as the base class (with responsibility for the things in read-md.o), and "rtx_reader" for the subclass (adding the things in read-rtl.o)? > > -static void > > -read_rtx_operand (rtx return_rtx, int idx) > > +rtx > > +rtx_reader::read_rtx_operand (rtx return_rtx, int idx) > > { > > RTX_CODE code = GET_CODE (return_rtx); > > const char *format_ptr = GET_RTX_FORMAT (code); > > > + > > + return return_rtx; > > } > > > > This appears to be an unrelated change. The return value needs to be > documented. Indeed. I'll split that change out into a followup. Thanks Dave