Re: [PATCH 1/3] Drop traversable_base::traverse method.

[Dodji Seketeli <dodji@seketeli.org>]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

Giuliano Procida <gprocida@google.com> a écrit:

> 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,


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: applied patch --]
[-- Type: text/x-patch, Size: 5979 bytes --]

From de7d3331ec7184b6fe89461be8cadf25326dbf62 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
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 <dodji@redhat.com>
---
 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> 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


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


-- 
		Dodji