From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by sourceware.org (Postfix) with ESMTPS id 1EAA4386F002 for ; Mon, 27 Jul 2020 16:03:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1EAA4386F002 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dodji@seketeli.org Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodji@seketeli.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 28BCC200005; Mon, 27 Jul 2020 16:03:29 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id DDAD8180090F; Mon, 27 Jul 2020 18:03:28 +0200 (CEST) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com, maennich@google.com Subject: Re: [PATCH 1/3] Drop traversable_base::traverse method. Organization: Me, myself and I References: <20200709164523.1578400-1-gprocida@google.com> <20200709182250.1677238-1-gprocida@google.com> <20200709182250.1677238-2-gprocida@google.com> X-Operating-System: Red Hat Enterprise Linux Workstation 7.8 Beta X-URL: http://www.seketeli.net/~dodji Date: Mon, 27 Jul 2020 18:03:28 +0200 In-Reply-To: <20200709182250.1677238-2-gprocida@google.com> (Giuliano Procida's message of "Thu, 9 Jul 2020 19:22:48 +0100") Message-ID: <87a6zlhrn3.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Jul 2020 16:03:37 -0000 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Giuliano Procida a =C3=A9crit: > The traverse method is defined in the traversable_base class but is > never used as the synonymous methods in the derived classes have > different argument types and so hide it. The derived classes have a traverse method which parameter type (ir_node_visitor and diff_node_visitor) derives from the type of the parameter of traversable_base::traverse (node_visitor_base). And that is by design. So yes they are different, but they are closely related. So I'd like to keep the traversable_base::traverse method at least as a comment, saying that types that extend traversable_base ought to declare a virtual traverse method which type extends node_visitor_base. I've done so in the attached patch I am applying. > The function can be removed and consequently the source file > abg-traverse.cc too. Rather than removing abg-traverse.cc, I'd pimpl-ify abg-traverse.h and move its inline definitions into abg-traverse.cc. This gets abg-traverse.h closer to a somewhat achieving some a{b,p}i stability in the future. It's a generic "backburner" goal that I have and I think removing this file gets me further from it. So I've done that in the patch as well. [...] Please find attached the patch I am applying to master. Cheers, --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Pimpl-ify-traversable_base-and-remove-its-unused-tra.patch Content-Description: applied patch >From de7d3331ec7184b6fe89461be8cadf25326dbf62 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Mon, 27 Jul 2020 16:55:09 +0200 Subject: [PATCH 1/2] Pimpl-ify traversable_base and remove its unused traverse method The traverse method is defined in the traversable_base class but is never used as the synonymous methods in the derived classes have different argument types and so hide it. It's never used because it's mostly intended as documentation for what the implementations of this interface should look like. Namely they should define a traversable method and its parameter type should derive from the parameter type of traversable_base::traverse. But apparently, clang's -Werror-overloaded-virtual is not happy about this. It flags it as an error. It's not. But hey, let's work-around it then. So this patch just comments that method out and document its intent. To make the change somewhat useful, this patch pimpl-ifies this abg-traverse.h header file to get us one step closer to some a{b,p}i stability. The definitions are moved into abg-traverse.cc. * include/abg-traverse.h (traversable_base::priv): Declare new type. (traverse_base::priv_sptr): Add pointer to private data member. (traverse_base::visiting_): Move this data member definition into traverse_base::priv. (traverse_base::{visiting, traverse_base, ~traverse_base}): Move definitions out-of-oline. (traverse_base::traverse): Comment out. * src/abg-traverse.cc (struct traversable_base::priv): Define new type. (traversable_base::{traversable_base, ~traversable_base, traverse, visiting}): Move these previous inline definitions here. Signed-off-by: Dodji Seketeli --- include/abg-traverse.h | 52 ++++++++++++++++++++++++-------------------------- src/abg-traverse.cc | 41 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/include/abg-traverse.h b/include/abg-traverse.h index a7170da..92a8a24 100644 --- a/include/abg-traverse.h +++ b/include/abg-traverse.h @@ -44,40 +44,30 @@ struct node_visitor_base /// The interface for types which are feeling social and want to be /// visited during the traversal of a hierarchy of nodes. +/// +/// It is expected that classes derived from traversable_base define a +/// traverse method specialised to the node *visitor* type. Such +/// methods should visit nodes recursively, calling +/// ir_node_visitor::visit for each node. The method should return +/// true until all nodes have been visited. class traversable_base { - bool visiting_; + struct priv; + typedef shared_ptr priv_sptr; + + priv_sptr priv_; protected: - /// This should returns false before and after the node has been - /// visiting. During the visiting of the node (and of its children) - /// this should return true. - /// - /// @return true if the current node is being visited. - bool - visiting() - {return visiting_;} + traversable_base(); - /// The traversing code should be responsible of calling this, not - /// the user code. - /// - /// This is the setter of the "visiting" flag of the node being - /// visited. If set to yes, it means the node is being visited. - /// False means either the node has not yet been visited, or it - /// has already been visited. - /// - /// @param f the new value of the "visiting" flag. - void - visiting(bool f) - {visiting_ = f;} + bool visiting() const; + + void visiting(bool f); public: - traversable_base() - : visiting_() - {} - virtual ~traversable_base() {} + virtual ~traversable_base(); /// This virtual method is overloaded and implemented by any single /// type which instance is going to be visited during the traversal @@ -92,8 +82,16 @@ public: /// /// @return true if traversed until the end of the type tree, false /// otherwise. - virtual bool traverse(node_visitor_base& v); -}; + /// + /// Note that each class that derives from this one and overloads + /// this method will have to define a type for its node visitor + /// argument (the one named v). That type will have to derive from + /// the node_visitor_base type. In that sense, this new overload + /// method will "hide" this one. That is why we are keeping this + /// method commented, for documentation purposes. + + // virtual bool traverse(node_visitor_base& v); +}; // end class traversable_base }// end namespace ir. }//end namespace abigail diff --git a/src/abg-traverse.cc b/src/abg-traverse.cc index 3fb87ea..a3cb797 100644 --- a/src/abg-traverse.cc +++ b/src/abg-traverse.cc @@ -35,9 +35,46 @@ namespace abigail namespace ir { +/// Private data type of the @ref traversable_base type. +struct traversable_base::priv +{ + bool visiting_; + + priv(bool visiting = false) + : visiting_(visiting) + {} +}; // end struct traversable_base::priv + +/// Default constructor of the @ref traversable_base type. +traversable_base::traversable_base() + :priv_(new priv) +{} + +/// Destructor of the @ref traversable_base type. +traversable_base::~traversable_base() +{} + +/// This should returns false before and after the node has been +/// visiting. During the visiting of the node (and of its children) +/// this should return true. +/// +/// @return true if the current node is being visited. bool -traversable_base::traverse(node_visitor_base&) -{return true;} +traversable_base::visiting() const +{return priv_->visiting_;} + +/// The traversing code should be responsible of calling this, not +/// the user code. +/// +/// This is the setter of the "visiting" flag of the node being +/// visited. If set to yes, it means the node is being visited. +/// False means either the node has not yet been visited, or it +/// has already been visited. +/// +/// @param f the new value of the "visiting" flag. +void +traversable_base::visiting(bool f) +{priv_->visiting_ = f;} }// end namaspace ir }// end namespace abigail -- 1.8.3.1 --=-=-= Content-Type: text/plain -- Dodji --=-=-=--