* [PATCH] Drop traversable_base::traverse method. @ 2020-07-09 16:45 Giuliano Procida 2020-07-09 18:22 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Giuliano Procida 0 siblings, 1 reply; 13+ messages in thread From: Giuliano Procida @ 2020-07-09 16:45 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich 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 function can be removed and consequently the source file abg-traverse.cc too. This removes a trigger of Clang's -Werror-overloaded-virtual which is enabled with -Wmost which is enabled by -Wall. * include/abg-traverse.h (class traversable_base): Add class-level comment about expected usage, drop declaration of traverse method. * src/Makefile.am: Remove mention of src/abg-traverse.cc. * src/abg-traverse.cc: Remove file as it only contains the unused traversable_base::traverse method. Signed-off-by: Giuliano Procida <gprocida@google.com> --- include/abg-traverse.h | 21 ++++++--------------- src/Makefile.am | 1 - src/abg-traverse.cc | 43 ------------------------------------------ 3 files changed, 6 insertions(+), 59 deletions(-) delete mode 100644 src/abg-traverse.cc diff --git a/include/abg-traverse.h b/include/abg-traverse.h index a7170dad..2cb84804 100644 --- a/include/abg-traverse.h +++ b/include/abg-traverse.h @@ -44,6 +44,12 @@ 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_; @@ -78,21 +84,6 @@ public: {} virtual ~traversable_base() {} - - /// This virtual method is overloaded and implemented by any single - /// type which instance is going to be visited during the traversal - /// of translation unit nodes. - /// - /// The method visits a given node and, for scopes, visits their - /// member nodes. Visiting a node means calling the - /// ir_node_visitor::visit method with the node passed as an - /// argument. - /// - /// @param v the visitor used during the traverse. - /// - /// @return true if traversed until the end of the type tree, false - /// otherwise. - virtual bool traverse(node_visitor_base& v); }; }// end namespace ir. diff --git a/src/Makefile.am b/src/Makefile.am index 1153a5f8..7684e373 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -13,7 +13,6 @@ endif libabigail_la_SOURCES = \ abg-internal.h \ -abg-traverse.cc \ abg-ir-priv.h \ abg-ir.cc \ abg-corpus-priv.h \ diff --git a/src/abg-traverse.cc b/src/abg-traverse.cc deleted file mode 100644 index 3fb87ea2..00000000 --- a/src/abg-traverse.cc +++ /dev/null @@ -1,43 +0,0 @@ -// -*- Mode: C++ -*- -// -// Copyright (C) 2013-2020 Red Hat, Inc. -// -// This file is part of the GNU Application Binary Interface Generic -// Analysis and Instrumentation Library (libabigail). This library is -// free software; you can redistribute it and/or modify it under the -// terms of the GNU Lesser General Public License as published by the -// Free Software Foundation; either version 3, or (at your option) any -// later version. - -// This library is distributed in the hope that it will be useful, but -// WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -// General Lesser Public License for more details. - -// You should have received a copy of the GNU Lesser General Public -// License along with this program; see the file COPYING-LGPLV3. If -// not, see <http://www.gnu.org/licenses/>. - -/// @file - -#include "abg-internal.h" -// <headers defining libabigail's API go under here> -ABG_BEGIN_EXPORT_DECLARATIONS - -#include "abg-traverse.h" - -ABG_END_EXPORT_DECLARATIONS -// </headers defining libabigail's API> - -namespace abigail -{ - -namespace ir -{ - -bool -traversable_base::traverse(node_visitor_base&) -{return true;} - -}// end namaspace ir -}// end namespace abigail -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/3] Bug 21485 - problems compiling with clang 2020-07-09 16:45 [PATCH] Drop traversable_base::traverse method Giuliano Procida @ 2020-07-09 18:22 ` Giuliano Procida 2020-07-09 18:22 ` [PATCH 1/3] Drop traversable_base::traverse method Giuliano Procida ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Giuliano Procida @ 2020-07-09 18:22 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich Hi. This short series fixes a known issue with Clang compilation of libabigail and error-overloaded-virtual warnings. The first patch removes an unused function. The second patch fixes a latent bug detected by the warning. The third patch enables the warning. Regards, Giuliano. Giuliano Procida (3): Drop traversable_base::traverse method. Fix inheritance of scope_decl::insert_member_decl Enable Clang's -Werror-overloaded-virtual. configure.ac | 6 ------ include/abg-ir.h | 3 +-- include/abg-traverse.h | 21 ++++++--------------- src/Makefile.am | 1 - src/abg-ir.cc | 2 +- src/abg-traverse.cc | 43 ------------------------------------------ 6 files changed, 8 insertions(+), 68 deletions(-) delete mode 100644 src/abg-traverse.cc -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] Drop traversable_base::traverse method. 2020-07-09 18:22 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Giuliano Procida @ 2020-07-09 18:22 ` Giuliano Procida 2020-07-10 16:26 ` Matthias Maennich 2020-07-27 16:03 ` Dodji Seketeli 2020-07-09 18:22 ` [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl Giuliano Procida ` (2 subsequent siblings) 3 siblings, 2 replies; 13+ messages in thread From: Giuliano Procida @ 2020-07-09 18:22 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich 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 function can be removed and consequently the source file abg-traverse.cc too. This removes a trigger of Clang's -Werror-overloaded-virtual which is enabled with -Wmost which is enabled by -Wall. * include/abg-traverse.h (class traversable_base): Add class-level comment about expected usage, drop declaration of traverse method. * src/Makefile.am: Remove mention of src/abg-traverse.cc. * src/abg-traverse.cc: Remove file as it only contains the unused traversable_base::traverse method. Signed-off-by: Giuliano Procida <gprocida@google.com> --- include/abg-traverse.h | 21 ++++++--------------- src/Makefile.am | 1 - src/abg-traverse.cc | 43 ------------------------------------------ 3 files changed, 6 insertions(+), 59 deletions(-) delete mode 100644 src/abg-traverse.cc diff --git a/include/abg-traverse.h b/include/abg-traverse.h index a7170dad..2cb84804 100644 --- a/include/abg-traverse.h +++ b/include/abg-traverse.h @@ -44,6 +44,12 @@ 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_; @@ -78,21 +84,6 @@ public: {} virtual ~traversable_base() {} - - /// This virtual method is overloaded and implemented by any single - /// type which instance is going to be visited during the traversal - /// of translation unit nodes. - /// - /// The method visits a given node and, for scopes, visits their - /// member nodes. Visiting a node means calling the - /// ir_node_visitor::visit method with the node passed as an - /// argument. - /// - /// @param v the visitor used during the traverse. - /// - /// @return true if traversed until the end of the type tree, false - /// otherwise. - virtual bool traverse(node_visitor_base& v); }; }// end namespace ir. diff --git a/src/Makefile.am b/src/Makefile.am index 1153a5f8..7684e373 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -13,7 +13,6 @@ endif libabigail_la_SOURCES = \ abg-internal.h \ -abg-traverse.cc \ abg-ir-priv.h \ abg-ir.cc \ abg-corpus-priv.h \ diff --git a/src/abg-traverse.cc b/src/abg-traverse.cc deleted file mode 100644 index 3fb87ea2..00000000 --- a/src/abg-traverse.cc +++ /dev/null @@ -1,43 +0,0 @@ -// -*- Mode: C++ -*- -// -// Copyright (C) 2013-2020 Red Hat, Inc. -// -// This file is part of the GNU Application Binary Interface Generic -// Analysis and Instrumentation Library (libabigail). This library is -// free software; you can redistribute it and/or modify it under the -// terms of the GNU Lesser General Public License as published by the -// Free Software Foundation; either version 3, or (at your option) any -// later version. - -// This library is distributed in the hope that it will be useful, but -// WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU -// General Lesser Public License for more details. - -// You should have received a copy of the GNU Lesser General Public -// License along with this program; see the file COPYING-LGPLV3. If -// not, see <http://www.gnu.org/licenses/>. - -/// @file - -#include "abg-internal.h" -// <headers defining libabigail's API go under here> -ABG_BEGIN_EXPORT_DECLARATIONS - -#include "abg-traverse.h" - -ABG_END_EXPORT_DECLARATIONS -// </headers defining libabigail's API> - -namespace abigail -{ - -namespace ir -{ - -bool -traversable_base::traverse(node_visitor_base&) -{return true;} - -}// end namaspace ir -}// end namespace abigail -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Drop traversable_base::traverse method. 2020-07-09 18:22 ` [PATCH 1/3] Drop traversable_base::traverse method Giuliano Procida @ 2020-07-10 16:26 ` Matthias Maennich 2020-07-27 16:03 ` Dodji Seketeli 1 sibling, 0 replies; 13+ messages in thread From: Matthias Maennich @ 2020-07-10 16:26 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team On Thu, Jul 09, 2020 at 07:22:48PM +0100, Giuliano Procida wrote: >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 function can be removed and consequently the source file >abg-traverse.cc too. > >This removes a trigger of Clang's -Werror-overloaded-virtual which is >enabled with -Wmost which is enabled by -Wall. > > * include/abg-traverse.h (class traversable_base): Add > class-level comment about expected usage, drop declaration of > traverse method. > * src/Makefile.am: Remove mention of src/abg-traverse.cc. > * src/abg-traverse.cc: Remove file as it only contains the > unused traversable_base::traverse method. > >Signed-off-by: Giuliano Procida <gprocida@google.com> Reviewed-by: Matthias Maennich <maennich@google.com> Cheers, Matthias >--- > include/abg-traverse.h | 21 ++++++--------------- > src/Makefile.am | 1 - > src/abg-traverse.cc | 43 ------------------------------------------ > 3 files changed, 6 insertions(+), 59 deletions(-) > delete mode 100644 src/abg-traverse.cc > >diff --git a/include/abg-traverse.h b/include/abg-traverse.h >index a7170dad..2cb84804 100644 >--- a/include/abg-traverse.h >+++ b/include/abg-traverse.h >@@ -44,6 +44,12 @@ 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_; >@@ -78,21 +84,6 @@ public: > {} > > virtual ~traversable_base() {} >- >- /// This virtual method is overloaded and implemented by any single >- /// type which instance is going to be visited during the traversal >- /// of translation unit nodes. >- /// >- /// The method visits a given node and, for scopes, visits their >- /// member nodes. Visiting a node means calling the >- /// ir_node_visitor::visit method with the node passed as an >- /// argument. >- /// >- /// @param v the visitor used during the traverse. >- /// >- /// @return true if traversed until the end of the type tree, false >- /// otherwise. >- virtual bool traverse(node_visitor_base& v); > }; > > }// end namespace ir. >diff --git a/src/Makefile.am b/src/Makefile.am >index 1153a5f8..7684e373 100644 >--- a/src/Makefile.am >+++ b/src/Makefile.am >@@ -13,7 +13,6 @@ endif > > libabigail_la_SOURCES = \ > abg-internal.h \ >-abg-traverse.cc \ > abg-ir-priv.h \ > abg-ir.cc \ > abg-corpus-priv.h \ >diff --git a/src/abg-traverse.cc b/src/abg-traverse.cc >deleted file mode 100644 >index 3fb87ea2..00000000 >--- a/src/abg-traverse.cc >+++ /dev/null >@@ -1,43 +0,0 @@ >-// -*- Mode: C++ -*- >-// >-// Copyright (C) 2013-2020 Red Hat, Inc. >-// >-// This file is part of the GNU Application Binary Interface Generic >-// Analysis and Instrumentation Library (libabigail). This library is >-// free software; you can redistribute it and/or modify it under the >-// terms of the GNU Lesser General Public License as published by the >-// Free Software Foundation; either version 3, or (at your option) any >-// later version. >- >-// This library is distributed in the hope that it will be useful, but >-// WITHOUT ANY WARRANTY; without even the implied warranty of >-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >-// General Lesser Public License for more details. >- >-// You should have received a copy of the GNU Lesser General Public >-// License along with this program; see the file COPYING-LGPLV3. If >-// not, see <http://www.gnu.org/licenses/>. >- >-/// @file >- >-#include "abg-internal.h" >-// <headers defining libabigail's API go under here> >-ABG_BEGIN_EXPORT_DECLARATIONS >- >-#include "abg-traverse.h" >- >-ABG_END_EXPORT_DECLARATIONS >-// </headers defining libabigail's API> >- >-namespace abigail >-{ >- >-namespace ir >-{ >- >-bool >-traversable_base::traverse(node_visitor_base&) >-{return true;} >- >-}// end namaspace ir >-}// end namespace abigail >-- >2.27.0.383.g050319c2ae-goog > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Drop traversable_base::traverse method. 2020-07-09 18:22 ` [PATCH 1/3] Drop traversable_base::traverse method Giuliano Procida 2020-07-10 16:26 ` Matthias Maennich @ 2020-07-27 16:03 ` Dodji Seketeli 1 sibling, 0 replies; 13+ messages in thread From: Dodji Seketeli @ 2020-07-27 16:03 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl 2020-07-09 18:22 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Giuliano Procida 2020-07-09 18:22 ` [PATCH 1/3] Drop traversable_base::traverse method Giuliano Procida @ 2020-07-09 18:22 ` Giuliano Procida 2020-07-10 16:27 ` Matthias Maennich 2020-07-27 16:14 ` Dodji Seketeli 2020-07-09 18:22 ` [PATCH 3/3] Enable Clang's -Werror-overloaded-virtual Giuliano Procida 2020-07-27 16:27 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Dodji Seketeli 3 siblings, 2 replies; 13+ messages in thread From: Giuliano Procida @ 2020-07-09 18:22 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich The classes class_decl, class_or_union and scope_decl derive from each other. The method insert_member_decl is declared virtual and defined in each of these. Unfortunately, it has different argument types in the base scope_decl class. Most calls to insert_member_decl are at a statically known class, but in insert_decl_into_scope the method is called via a scope_decl pointer. There is the possibility that this could be a type derived from scope_decl rather than scope_decl itself, in which case the base method would be called, not as intended. This commit adjusts the type of the member argument to scope_decl::insert_member_decl to match the other two classes and eliminates the last trigger of Clang's -Werror-overloaded-virtual. * include/abg-ir.h (scope_decl::insert_member_decl): Change type of member argument from const decl_base_sptr& to plain decl_base_sptr. * src/abg-ir.cc (scope_decl::insert_member_decl): Likewise. Signed-off-by: Giuliano Procida <gprocida@google.com> --- include/abg-ir.h | 3 +-- src/abg-ir.cc | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/abg-ir.h b/include/abg-ir.h index c2b66c4c..ea6a7ce4 100644 --- a/include/abg-ir.h +++ b/include/abg-ir.h @@ -1653,8 +1653,7 @@ protected: add_member_decl(const decl_base_sptr& member); virtual decl_base_sptr - insert_member_decl(const decl_base_sptr& member, - declarations::iterator before); + insert_member_decl(decl_base_sptr member, declarations::iterator before); virtual void remove_member_decl(decl_base_sptr member); diff --git a/src/abg-ir.cc b/src/abg-ir.cc index a434ec69..a257cf67 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -6081,7 +6081,7 @@ scope_decl::add_member_decl(const decl_base_sptr& member) /// @param before an interator pointing to the element before which /// the new member should be inserted. decl_base_sptr -scope_decl::insert_member_decl(const decl_base_sptr& member, +scope_decl::insert_member_decl(decl_base_sptr member, declarations::iterator before) { ABG_ASSERT(!member->get_scope()); -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl 2020-07-09 18:22 ` [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl Giuliano Procida @ 2020-07-10 16:27 ` Matthias Maennich 2020-07-27 16:14 ` Dodji Seketeli 1 sibling, 0 replies; 13+ messages in thread From: Matthias Maennich @ 2020-07-10 16:27 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team On Thu, Jul 09, 2020 at 07:22:49PM +0100, Giuliano Procida wrote: >The classes class_decl, class_or_union and scope_decl derive from each >other. The method insert_member_decl is declared virtual and defined >in each of these. Unfortunately, it has different argument types in >the base scope_decl class. > >Most calls to insert_member_decl are at a statically known class, but >in insert_decl_into_scope the method is called via a scope_decl >pointer. There is the possibility that this could be a type derived >from scope_decl rather than scope_decl itself, in which case the base >method would be called, not as intended. > >This commit adjusts the type of the member argument to >scope_decl::insert_member_decl to match the other two classes and >eliminates the last trigger of Clang's -Werror-overloaded-virtual. > > * include/abg-ir.h (scope_decl::insert_member_decl): Change > type of member argument from const decl_base_sptr& to plain > decl_base_sptr. > * src/abg-ir.cc (scope_decl::insert_member_decl): Likewise. > >Signed-off-by: Giuliano Procida <gprocida@google.com> Reviewed-by: Matthias Maennich <maennich@google.com> Cheers, Matthias >--- > include/abg-ir.h | 3 +-- > src/abg-ir.cc | 2 +- > 2 files changed, 2 insertions(+), 3 deletions(-) > >diff --git a/include/abg-ir.h b/include/abg-ir.h >index c2b66c4c..ea6a7ce4 100644 >--- a/include/abg-ir.h >+++ b/include/abg-ir.h >@@ -1653,8 +1653,7 @@ protected: > add_member_decl(const decl_base_sptr& member); > > virtual decl_base_sptr >- insert_member_decl(const decl_base_sptr& member, >- declarations::iterator before); >+ insert_member_decl(decl_base_sptr member, declarations::iterator before); > > virtual void > remove_member_decl(decl_base_sptr member); >diff --git a/src/abg-ir.cc b/src/abg-ir.cc >index a434ec69..a257cf67 100644 >--- a/src/abg-ir.cc >+++ b/src/abg-ir.cc >@@ -6081,7 +6081,7 @@ scope_decl::add_member_decl(const decl_base_sptr& member) > /// @param before an interator pointing to the element before which > /// the new member should be inserted. > decl_base_sptr >-scope_decl::insert_member_decl(const decl_base_sptr& member, >+scope_decl::insert_member_decl(decl_base_sptr member, > declarations::iterator before) > { > ABG_ASSERT(!member->get_scope()); >-- >2.27.0.383.g050319c2ae-goog > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl 2020-07-09 18:22 ` [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl Giuliano Procida 2020-07-10 16:27 ` Matthias Maennich @ 2020-07-27 16:14 ` Dodji Seketeli 1 sibling, 0 replies; 13+ messages in thread From: Dodji Seketeli @ 2020-07-27 16:14 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich Giuliano Procida <gprocida@google.com> a écrit: > The classes class_decl, class_or_union and scope_decl derive from each > other. The method insert_member_decl is declared virtual and defined > in each of these. Unfortunately, it has different argument types in > the base scope_decl class. > > Most calls to insert_member_decl are at a statically known class, but > in insert_decl_into_scope the method is called via a scope_decl > pointer. There is the possibility that this could be a type derived > from scope_decl rather than scope_decl itself, in which case the base > method would be called, not as intended. > > This commit adjusts the type of the member argument to > scope_decl::insert_member_decl to match the other two classes and > eliminates the last trigger of Clang's -Werror-overloaded-virtual. > > * include/abg-ir.h (scope_decl::insert_member_decl): Change > type of member argument from const decl_base_sptr& to plain > decl_base_sptr. > * src/abg-ir.cc (scope_decl::insert_member_decl): Likewise. > > Signed-off-by: Giuliano Procida <gprocida@google.com> Applied to master, thanks! Cheers, -- Dodji ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] Enable Clang's -Werror-overloaded-virtual. 2020-07-09 18:22 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Giuliano Procida 2020-07-09 18:22 ` [PATCH 1/3] Drop traversable_base::traverse method Giuliano Procida 2020-07-09 18:22 ` [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl Giuliano Procida @ 2020-07-09 18:22 ` Giuliano Procida 2020-07-09 18:51 ` [PATCH v2 " Giuliano Procida 2020-07-27 16:27 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Dodji Seketeli 3 siblings, 1 reply; 13+ messages in thread From: Giuliano Procida @ 2020-07-09 18:22 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich Just to see what's still there. Signed-off-by: Giuliano Procida <gprocida@google.com> --- configure.ac | 6 ------ 1 file changed, 6 deletions(-) diff --git a/configure.ac b/configure.ac index 225ac38f..904a726e 100644 --- a/configure.ac +++ b/configure.ac @@ -646,12 +646,6 @@ if test x$ENABLE_UBSAN = xyes; then CXXFLAGS="$CXXFLAGS -fsanitize=undefined" fi -dnl Check if we compile with clang and set flags accordingly -dnl TODO: fix that warning (spoiler: not trivial) -if `$CXX -v 2>&1 | grep 'clang version' > /dev/null 2>&1`; then - CXXFLAGS="$CXXFLAGS -Wno-error-overloaded-virtual" -fi - dnl Check if several decls and constant are defined in dependant dnl libraries HAS_EM_AARCH64=no -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] Enable Clang's -Werror-overloaded-virtual. 2020-07-09 18:22 ` [PATCH 3/3] Enable Clang's -Werror-overloaded-virtual Giuliano Procida @ 2020-07-09 18:51 ` Giuliano Procida 2020-07-10 16:28 ` Matthias Maennich 2020-07-27 16:26 ` Dodji Seketeli 0 siblings, 2 replies; 13+ messages in thread From: Giuliano Procida @ 2020-07-09 18:51 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich Ths warning is no longer triggered and can be reenabled. * configure.ac: Remove the special clause that disabled -Werror-overloaded-virtual for Clang builds. Signed-off-by: Giuliano Procida <gprocida@google.com> --- configure.ac | 6 ------ 1 file changed, 6 deletions(-) diff --git a/configure.ac b/configure.ac index 225ac38f..904a726e 100644 --- a/configure.ac +++ b/configure.ac @@ -646,12 +646,6 @@ if test x$ENABLE_UBSAN = xyes; then CXXFLAGS="$CXXFLAGS -fsanitize=undefined" fi -dnl Check if we compile with clang and set flags accordingly -dnl TODO: fix that warning (spoiler: not trivial) -if `$CXX -v 2>&1 | grep 'clang version' > /dev/null 2>&1`; then - CXXFLAGS="$CXXFLAGS -Wno-error-overloaded-virtual" -fi - dnl Check if several decls and constant are defined in dependant dnl libraries HAS_EM_AARCH64=no -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] Enable Clang's -Werror-overloaded-virtual. 2020-07-09 18:51 ` [PATCH v2 " Giuliano Procida @ 2020-07-10 16:28 ` Matthias Maennich 2020-07-27 16:26 ` Dodji Seketeli 1 sibling, 0 replies; 13+ messages in thread From: Matthias Maennich @ 2020-07-10 16:28 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team On Thu, Jul 09, 2020 at 07:51:34PM +0100, Giuliano Procida wrote: >Ths warning is no longer triggered and can be reenabled. > > * configure.ac: Remove the special clause that disabled > -Werror-overloaded-virtual for Clang builds. > >Signed-off-by: Giuliano Procida <gprocida@google.com> Reviewed-by: Matthias Maennich <maennich@google.com> Cheers, Matthias >--- > configure.ac | 6 ------ > 1 file changed, 6 deletions(-) > >diff --git a/configure.ac b/configure.ac >index 225ac38f..904a726e 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -646,12 +646,6 @@ if test x$ENABLE_UBSAN = xyes; then > CXXFLAGS="$CXXFLAGS -fsanitize=undefined" > fi > >-dnl Check if we compile with clang and set flags accordingly >-dnl TODO: fix that warning (spoiler: not trivial) >-if `$CXX -v 2>&1 | grep 'clang version' > /dev/null 2>&1`; then >- CXXFLAGS="$CXXFLAGS -Wno-error-overloaded-virtual" >-fi >- > dnl Check if several decls and constant are defined in dependant > dnl libraries > HAS_EM_AARCH64=no >-- >2.27.0.383.g050319c2ae-goog > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] Enable Clang's -Werror-overloaded-virtual. 2020-07-09 18:51 ` [PATCH v2 " Giuliano Procida 2020-07-10 16:28 ` Matthias Maennich @ 2020-07-27 16:26 ` Dodji Seketeli 1 sibling, 0 replies; 13+ messages in thread From: Dodji Seketeli @ 2020-07-27 16:26 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich Giuliano Procida <gprocida@google.com> a écrit: > Ths warning is no longer triggered and can be reenabled. > > * configure.ac: Remove the special clause that disabled > -Werror-overloaded-virtual for Clang builds. > > Signed-off-by: Giuliano Procida <gprocida@google.com> Applied to master, thanks! Cheers, -- Dodji ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] Bug 21485 - problems compiling with clang 2020-07-09 18:22 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Giuliano Procida ` (2 preceding siblings ...) 2020-07-09 18:22 ` [PATCH 3/3] Enable Clang's -Werror-overloaded-virtual Giuliano Procida @ 2020-07-27 16:27 ` Dodji Seketeli 3 siblings, 0 replies; 13+ messages in thread From: Dodji Seketeli @ 2020-07-27 16:27 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich Giuliano Procida <gprocida@google.com> a écrit: > Hi. > > This short series fixes a known issue with Clang compilation of > libabigail and error-overloaded-virtual warnings. > > The first patch removes an unused function. > The second patch fixes a latent bug detected by the warning. > The third patch enables the warning. This should hopefully be addressed now. So I guess the bug can be closed now. Thanks! Cheers, -- Dodji ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-27 16:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-09 16:45 [PATCH] Drop traversable_base::traverse method Giuliano Procida 2020-07-09 18:22 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Giuliano Procida 2020-07-09 18:22 ` [PATCH 1/3] Drop traversable_base::traverse method Giuliano Procida 2020-07-10 16:26 ` Matthias Maennich 2020-07-27 16:03 ` Dodji Seketeli 2020-07-09 18:22 ` [PATCH 2/3] Fix inheritance of scope_decl::insert_member_decl Giuliano Procida 2020-07-10 16:27 ` Matthias Maennich 2020-07-27 16:14 ` Dodji Seketeli 2020-07-09 18:22 ` [PATCH 3/3] Enable Clang's -Werror-overloaded-virtual Giuliano Procida 2020-07-09 18:51 ` [PATCH v2 " Giuliano Procida 2020-07-10 16:28 ` Matthias Maennich 2020-07-27 16:26 ` Dodji Seketeli 2020-07-27 16:27 ` [PATCH 0/3] Bug 21485 - problems compiling with clang Dodji Seketeli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).