[PATCH v1 10/10] dwarf-reader: fix recursion in expr_result::operator&

[PATCH v1 10/10] dwarf-reader: fix recursion in expr_result::operator&

[Matthias Maennich via libabigail]

operator& was implemented in terms of itself, leading to infinite
recursion. Fix that by implementing it in terms of &='ing the const
value. That is consistent with all other ?= operators.

 * src/abg-dwarf-reader.cc: fix expr_result::operator&

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-dwarf-reader.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index d19849d7fff7..0999f12f1532 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -2626,7 +2626,7 @@ public:
   operator&(const expr_result& o)
   {
     expr_result r(*this);
-    r.const_value_ = *this & o;
+    r.const_value_ &= o.const_value_;
     r.is_const_ = r.is_const_ && o.is_const_;
     return r;
   }
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 08/10] ir: drop unused data members from {environment,qualified_name}_setter

[Matthias Maennich via libabigail]

The data members environment_setter::artifact_ and
qualified_name_setter::node_ are not used and can therefore be dropped
along with all their references.

 * src/abg-ir.cc: drop unused data members

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-ir.cc | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 1a6aed7ff0e4..192c52ab9c39 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -56,14 +56,11 @@ namespace
 /// to a new environment.
 class environment_setter : public abigail::ir::ir_node_visitor
 {
-  abigail::ir::type_or_decl_base* artifact_;
   const abigail::ir::environment* env_;
 
 public:
-  environment_setter(abigail::ir::type_or_decl_base*	a,
-		     const abigail::ir::environment*	env)
-    : artifact_(a),
-      env_(env)
+  environment_setter(const abigail::ir::environment* env)
+    : env_(env)
   {}
 
   /// This function is called on each sub-tree node that is a
@@ -112,13 +109,8 @@ public:
 /// function update_qualified_name().
 class qualified_name_setter : public abigail::ir::ir_node_visitor
 {
-  abigail::ir::decl_base* node_;
 
 public:
-  qualified_name_setter(abigail::ir::decl_base* node)
-    : node_(node)
-  {}
-
   bool
   do_update(abigail::ir::decl_base* d);
 
@@ -2803,7 +2795,7 @@ set_environment_for_artifact(type_or_decl_base* artifact,
 {
   ABG_ASSERT(artifact && env);
 
-  ::environment_setter s(artifact, env);
+  ::environment_setter s(env);
   artifact->traverse(s);
 }
 
@@ -4988,7 +4980,7 @@ peel_typedef_pointer_or_reference_type(const type_base* type,
 static void
 update_qualified_name(decl_base * d)
 {
-  ::qualified_name_setter setter(d);
+  ::qualified_name_setter setter;
   d->traverse(setter);
 }
 
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 06/10] viz-dot: remove unused members from dot

[Matthias Maennich via libabigail]

_M_canvas and _M_typo are unused within "dot". Remove them and all
references.

 * include/abg-viz-dot.h: remove unused data members from 'dot'

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 include/abg-viz-dot.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/abg-viz-dot.h b/include/abg-viz-dot.h
index a75d8689729c..660862d172d1 100644
--- a/include/abg-viz-dot.h
+++ b/include/abg-viz-dot.h
@@ -111,17 +111,13 @@ struct dot
 private:
 
   const std::string    	_M_title;
-  const canvas&	       	_M_canvas;
-  const typography&    	_M_typo;	
 
   std::ostringstream   	_M_sstream;
   
 public:
 
-  dot(const std::string &__title,
-      const canvas& __cv = ansi_letter_canvas,
-      const typography& __typo = arial_typo) 
-  : _M_title(__title), _M_canvas(__cv), _M_typo(__typo)
+  dot(const std::string &__title)
+  : _M_title(__title)
   { }
   
   // Empty when the output buffer is.
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v1 00/10] Fix warnings emitted when building with Clang

[Dodji Seketeli]

Hello Matthias,

"Matthias Maennich via libabigail" <libabigail@sourceware.org> a écrit:

> Hi!
>
> While exploring the code base I was using clang-7 to compile. This patch series
> addresses warnings that were emitted when doing so. I split them up in single
> commits (grouped by type) as some might be subject to discussion.
>
> One warning category (-Woverloaded-virtual) is still outstanding. That might
> require more work / refactoring to solve.

Thanks a lot for the series!

The patch set looks good to me, obviously.

So I just pushed it!

Thanks again.

Cheers,

-- 
		Dodji

[PATCH v1 02/10] abilint: fix return types bool -> int

[Matthias Maennich via libabigail]

Returning bool literals from main can be misleading. Returning booleans
maps to (by convention):

   return false -> converted to 0 -> rc=0 considered SUCCESS
   return true  -> converted to 1 -> rc=1 considered FAILURE

Compiling with clang also emits:

  abilint.cc:258:7: warning: bool literal returned from 'main' [-Wmain]
      return true;
      ^      ~~~~

The issues can be addressed by consistently returning integers as also
done in all other mains across the project.

Same issue applies to print-diff-tree.cc.

 * tools/abilint.cc: return int in main rather than bool.
 * tests/print-diff-tree.cc: Likewise.

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 tests/print-diff-tree.cc |  2 +-
 tools/abilint.cc         | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tests/print-diff-tree.cc b/tests/print-diff-tree.cc
index aefe3e7511a9..fe8f37879c78 100644
--- a/tests/print-diff-tree.cc
+++ b/tests/print-diff-tree.cc
@@ -106,7 +106,7 @@ main(int argc, char* argv[])
     {
       cerr << "unrecognized option\n"
 	"try the --help option for more information\n";
-      return false;
+      return 1;
     }
 
   if (opts.display_help)
diff --git a/tools/abilint.cc b/tools/abilint.cc
index e51909d8172d..8798a24ec256 100644
--- a/tools/abilint.cc
+++ b/tools/abilint.cc
@@ -255,7 +255,7 @@ main(int argc, char* argv[])
 	emit_prefix(argv[0], cerr)
 	  << "unrecognized option: " << opts.wrong_option << "\n";
       display_usage(argv[0], cerr);
-      return true;
+      return 1;
     }
 
   if (opts.display_version)
@@ -266,13 +266,13 @@ main(int argc, char* argv[])
     }
 
   if (!maybe_check_suppression_files(opts))
-    return true;
+    return 1;
 
   abigail::ir::environment_sptr env(new abigail::ir::environment);
   if (opts.read_from_stdin)
     {
       if (!cin.good())
-	return true;
+	return 1;
 
       if (opts.read_tu)
 	{
@@ -283,12 +283,12 @@ main(int argc, char* argv[])
 	    {
 	      emit_prefix(argv[0], cerr)
 		<< "failed to read the ABI instrumentation from stdin\n";
-	      return true;
+	      return 1;
 	    }
 
 	  if (!opts.noout)
 	    write_translation_unit(*tu, 0, cout);
-	  return false;
+	  return 0;
 	}
       else
 	{
@@ -300,13 +300,13 @@ main(int argc, char* argv[])
 	  corpus_sptr corp = abigail::xml_reader::read_corpus_from_input(*ctxt);
 	  if (!opts.noout)
 	    write_corpus(corp, /*indent=*/0, cout);
-	  return false;
+	  return 0;
 	}
     }
   else if (!opts.file_path.empty())
     {
       if (!check_file(opts.file_path, cerr, argv[0]))
-	return true;
+	return 1;
       abigail::translation_unit_sptr tu;
       abigail::corpus_sptr corp;
       abigail::dwarf_reader::status s = abigail::dwarf_reader::STATUS_OK;
@@ -318,7 +318,7 @@ main(int argc, char* argv[])
 	case abigail::tools_utils::FILE_TYPE_UNKNOWN:
 	  emit_prefix(argv[0], cerr)
 	    << "Unknown file type given in input: " << opts.file_path;
-	  return true;
+	  return 1;
 	case abigail::tools_utils::FILE_TYPE_NATIVE_BI:
 	  tu = read_translation_unit_from_file(opts.file_path, env.get());
 	  break;
@@ -392,7 +392,7 @@ main(int argc, char* argv[])
 		  << opts.file_path
 		  << "\n";
 	    }
-	  return true;
+	  return 1;
 	}
 
       using abigail::tools_utils::temp_file;
@@ -402,7 +402,7 @@ main(int argc, char* argv[])
       if (!tmp_file)
 	{
 	  emit_prefix(argv[0], cerr) << "failed to create temporary file\n";
-	  return true;
+	  return 1;
 	}
 
       std::iostream& of = tmp_file->get_stream();
@@ -465,8 +465,8 @@ main(int argc, char* argv[])
 	    is_ok = false;
 	}
 
-      return !is_ok;
+      return is_ok ? 0 : 1;
     }
 
-  return true;
+  return 1;
 }
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 04/10] diff-utils: point: fix postfix decrement/increment operator

[Matthias Maennich via libabigail]

The postfix increment / decrement operators were implemented by calling
themselves recursively. Fix that by implementing these in terms of their
prefix counter parts.

 * include/abg-diff-utils.h: fix postfix dec/inc operator

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 include/abg-diff-utils.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/abg-diff-utils.h b/include/abg-diff-utils.h
index 4e65fe8fe34b..1c71185e3b03 100644
--- a/include/abg-diff-utils.h
+++ b/include/abg-diff-utils.h
@@ -186,7 +186,7 @@ public:
   operator--(int)
   {
     point tmp(*this);
-    (*this)--;
+    --(*this);
     return tmp;
   }
 
@@ -194,7 +194,7 @@ public:
   operator++(int)
   {
     point tmp(*this);
-    (*this)++;
+    ++(*this);
     return tmp;
   }
 
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 00/10] Fix warnings emitted when building with Clang

[Matthias Maennich via libabigail]

Hi!

While exploring the code base I was using clang-7 to compile. This patch series
addresses warnings that were emitted when doing so. I split them up in single
commits (grouped by type) as some might be subject to discussion.

One warning category (-Woverloaded-virtual) is still outstanding. That might
require more work / refactoring to solve.

Cheers,
Matthias

===

Matthias Maennich (10):
  abg-fwd.h: fix mismatched tags for ir_node_visitor
  abilint: fix return types bool -> int
  abg-reader: clarify boolean use of assignment
  diff-utils: point: fix postfix decrement/increment operator
  add missing virtual destructors
  viz-dot: remove unused members from dot
  suppressions: drop unused parameter from type_is_suppressed
  ir: drop unused data members from {environment,qualified_name}_setter
  distinct_diff: avoid expression with side effects within typeid
  dwarf-reader: fix recursion in expr_result::operator&

 include/abg-comparison.h   |  4 ++++
 include/abg-corpus.h       |  2 ++
 include/abg-diff-utils.h   |  4 ++--
 include/abg-fwd.h          |  2 +-
 include/abg-reporter.h     |  1 +
 include/abg-traverse.h     |  3 +++
 include/abg-viz-dot.h      |  8 ++------
 src/abg-comparison.cc      |  3 ++-
 src/abg-dwarf-reader.cc    |  2 +-
 src/abg-ir.cc              | 16 ++++------------
 src/abg-reader.cc          |  2 +-
 src/abg-suppression-priv.h |  3 +--
 tests/print-diff-tree.cc   |  2 +-
 tools/abilint.cc           | 24 ++++++++++++------------
 14 files changed, 37 insertions(+), 39 deletions(-)

-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 01/10] abg-fwd.h: fix mismatched tags for ir_node_visitor

[Matthias Maennich via libabigail]

ir_node_visitor is defined as `class` in include/abg-ir.h:4429 and
should therefore also be forward-declared as such.

 * include/abg-fwd.h: forward-declare ir_node_visitor as class

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 include/abg-fwd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/abg-fwd.h b/include/abg-fwd.h
index 4625841d5c0a..82d91fe245e9 100644
--- a/include/abg-fwd.h
+++ b/include/abg-fwd.h
@@ -87,7 +87,7 @@ typedef shared_ptr<corpus_group> corpus_group_sptr;
 
 // Forward declarations for ir.
 
-struct ir_node_visitor;
+class  ir_node_visitor;
 
 struct ir_traversable_base;
 
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 05/10] add missing virtual destructors

[Matthias Maennich via libabigail]

Several virtual desctructors were missing. Even though there might not
have been actual leaks or similar bugs, it is worth fixing these
locations as they might lead to bugs in the future.

Clang also warns at these locations:

  warning: delete called on non-final 'abigail::ir::corpus' that has virtual
  functions but non-virtual destructor [-Wdelete-non-virtual-dtor]

 * include/abg-comparison.h: add virtual destructor for corpus_diff and diff_node_visitor
 * include/abg-corpus.h: add virtual destructor for corpus
 * include/abg-reporter.h: add virtual destructor for reporter_base
 * include/abg-traverse.h: add virtual destructor for traversable_base

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 include/abg-comparison.h | 4 ++++
 include/abg-corpus.h     | 2 ++
 include/abg-reporter.h   | 1 +
 include/abg-traverse.h   | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index ba4339c9b434..cf2fa4e151a7 100644
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -2278,6 +2278,8 @@ public:
 
   class diff_stats;
 
+  virtual ~corpus_diff() {}
+
   /// A convenience typedef for a shared pointer to @ref diff_stats
   typedef shared_ptr<diff_stats> diff_stats_sptr;
 
@@ -2544,6 +2546,8 @@ public:
 
   diff_node_visitor();
 
+  virtual ~diff_node_visitor() {}
+
   diff_node_visitor(visiting_kind k);
 
   visiting_kind
diff --git a/include/abg-corpus.h b/include/abg-corpus.h
index 258056e45222..1a25b4d1458d 100644
--- a/include/abg-corpus.h
+++ b/include/abg-corpus.h
@@ -78,6 +78,8 @@ public:
 
   corpus(ir::environment*, const string& path= "");
 
+  virtual ~corpus() {}
+
   const environment*
   get_environment() const;
 
diff --git a/include/abg-reporter.h b/include/abg-reporter.h
index 4085aeb82bcd..55cb8cc79ddc 100644
--- a/include/abg-reporter.h
+++ b/include/abg-reporter.h
@@ -141,6 +141,7 @@ public:
   report(const corpus_diff& d, ostream& out,
 	 const string& indent = "") const = 0;
 
+  virtual ~reporter_base() {}
 }; //end class reporter_base
 
 class default_reporter;
diff --git a/include/abg-traverse.h b/include/abg-traverse.h
index 5d395c2a439f..848c71b2484c 100644
--- a/include/abg-traverse.h
+++ b/include/abg-traverse.h
@@ -76,6 +76,9 @@ public:
   traversable_base()
     : visiting_()
   {}
+
+  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.
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 09/10] distinct_diff: avoid expression with side effects within typeid

[Matthias Maennich via libabigail]

When compiling with clang, the following warning is emitted:

abg-comparison.cc:2674:17: warning: expression with side effects will be
evaluated despite being used as an operand to 'typeid' [-Wpotentially-evaluated-expression]
  return typeid(*first.get()) != typeid(*second.get());
                ^

Mitigate that warning by moving potential side effects out of typeid.

 * src/abg-comparison.cc: fix clang warning "potentially-evaluated-expression"

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-comparison.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 8a66367804d7..ed5e05883c42 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -2671,7 +2671,8 @@ distinct_diff::entities_are_of_distinct_kinds(type_or_decl_base_sptr first,
   if (first == second)
     return false;
 
-  return typeid(*first.get()) != typeid(*second.get());
+  const type_or_decl_base &f = *first, &s = *second;
+  return typeid(f) != typeid(s);
 }
 
 /// @return true if the two subjects of the diff are different, false
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 07/10] suppressions: drop unused parameter from type_is_suppressed

[Matthias Maennich via libabigail]

Drop 'require_drop_property' as it is not used at all.
That fixes a clang warning.

 * include/abg-suppression-priv.h: drop unused argument from type_is_suppressed

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-suppression-priv.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h
index 45e679351f4d..f157c35819e2 100644
--- a/src/abg-suppression-priv.h
+++ b/src/abg-suppression-priv.h
@@ -807,8 +807,7 @@ template <typename ReadContextType>
 bool
 type_is_suppressed(const ReadContextType&	ctxt,
 		   const string&		type_name,
-		   const location&		type_location,
-		   bool require_drop_property = false)
+		   const location&		type_location)
 {
   bool type_is_private = false;
   return type_is_suppressed(ctxt, type_name, type_location, type_is_private);
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v1 03/10] abg-reader: clarify boolean use of assignment

[Matthias Maennich via libabigail]

When compiling with clang, the following warning is emitted:

abg-reader.cc:1981:15: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
  while (corp = read_corpus_from_input(ctxt))
         ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That is certainly a common pitfall and can be mitigated by placing
parentheses around the assignment.

 * src/abg-reader.cc: clarify boolean use of assignment

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-reader.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 2204e153e7c1..69329d1f1cb5 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -1978,7 +1978,7 @@ read_corpus_group_from_input(read_context& ctxt)
   ctxt.set_corpus_node(node);
 
   corpus_sptr corp;
-  while (corp = read_corpus_from_input(ctxt))
+  while ((corp = read_corpus_from_input(ctxt)))
     ctxt.get_corpus_group()->add_corpus(corp);
 
   xmlTextReaderNext(reader.get());
-- 
2.21.0.392.gf8f6787159e-goog