[PATCH 0/3] clang-tidy error and warning fixes

[PATCH 0/3] clang-tidy error and warning fixes

[Giuliano Procida]

Hi Dodji.

We (also) build libabigail using Google's internal tooling and one
thing we get for free is clang-tidy's report on the code. The internal
branch of libabigail is essentially a subset of mm-next, so the
figures below may not be accurate for master.

The current counts (for */*.{h,cc}) are:

      2 clang-diagnostic-error
      2 readability-redundant-smartptr-get
     14 performance-unnecessary-copy-initialization
     21 readability-container-size-empty
     49 bugprone-argument-comment
     50 misc-unused-using-decls
     54 clang-diagnostic-shadow-field
     66 readability-inconsistent-declaration-parameter-name

The commits in this series address the first 3 categories above.

I'm proceeding on the assumption that there is value in reducing the
counts to zero. However, it's possible that certain categories may not
be aligned with an idealised libabigail coding standard. Do let us
know. Thank you!

Regards,
Giuliano.

Giuliano Procida (3):
  abg-corpus-priv.h: include abg-corpus.h
  Fix readability-redundant-smartptr-get warnings
  Fix performance-unnecessary-copy-initialization warnings

 include/abg-diff-utils.h    |  2 +-
 src/abg-comparison-priv.h   |  3 ++-
 src/abg-corpus-priv.h       |  1 +
 src/abg-default-reporter.cc |  4 ++--
 src/abg-dwarf-reader.cc     |  2 +-
 src/abg-ir.cc               | 11 ++++++-----
 src/abg-suppression.cc      |  8 ++++----
 tools/abipkgdiff.cc         |  2 +-
 tools/kmidiff.cc            |  4 ++--
 9 files changed, 20 insertions(+), 17 deletions(-)

-- 
2.28.0.402.g5ffc5be6b7-goog

[PATCH 1/3] abg-corpus-priv.h: include abg-corpus.h

[Giuliano Procida]

This commit eliminates a clang diagnostic error that can occur if the
header abg-corpus-priv.h alone is compiled as the type corpus was
incomplete at the point various nested types were defined.

	* src/abg-corpus-priv.h: Include abg-corpus.h.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-corpus-priv.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
index ee8c82396..d8b75cb26 100644
--- a/src/abg-corpus-priv.h
+++ b/src/abg-corpus-priv.h
@@ -30,6 +30,7 @@
 #define __ABG_CORPUS_PRIV_H__
 
 #include "abg-internal.h"
+#include "abg-corpus.h"
 #include "abg-regex.h"
 #include "abg-sptr-utils.h"
 
-- 
2.28.0.402.g5ffc5be6b7-goog

[PATCH 2/3] Fix readability-redundant-smartptr-get warnings

[Giuliano Procida]

clang-tidy's readability-redundant-smartptr-get warning is triggered
when it detects a redundant "get" of various smart pointer types in
boolean context. This commit fixes these warnings.

	* tools/kmidiff.cc (main): Remove redundant .get() when
	testing smart pointers.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 tools/kmidiff.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/kmidiff.cc b/tools/kmidiff.cc
index 2840eb086..41584d3ae 100644
--- a/tools/kmidiff.cc
+++ b/tools/kmidiff.cc
@@ -416,7 +416,7 @@ main(int argc, char* argv[])
       file_type ftype = guess_file_type(opts.kernel_dist_root1);
       if (ftype == FILE_TYPE_DIR)
 	{
-	  debug_info_root_dir = opts.di_root_path1.get()
+	  debug_info_root_dir = opts.di_root_path1
 	    ? opts.di_root_path1.get()
 	    : "";
 
@@ -443,7 +443,7 @@ main(int argc, char* argv[])
       file_type ftype = guess_file_type(opts.kernel_dist_root2);
       if (ftype == FILE_TYPE_DIR)
 	{
-	  debug_info_root_dir = opts.di_root_path2.get()
+	  debug_info_root_dir = opts.di_root_path2
 	    ? opts.di_root_path2.get()
 	    : "";
 	  group2 =
-- 
2.28.0.402.g5ffc5be6b7-goog

[PATCH 3/3] Fix performance-unnecessary-copy-initialization warnings

[Giuliano Procida]

clang-tidy's performance-unnecessary-copy-initialization warning is
triggered when it detects unncessary copying of objects. In the case
of the shared_ptr types used by libabigail, the extra cost is
primarily that of manipulating reference counts.

This commit addresses these warnings. Note that while they are
unlikely to be genuine performance issues, eliminating them means
we'll be able to spot new issues more easily.

	* include/abg-diff-utils.h (compute_diff): Take const
	reference to point instead of copying.
	* src/abg-comparison-priv.h (elf_symbol_comp::operator()):
	Take const references to strings instead of copying.
	* src/abg-default-reporter.cc (default_reporter::report): Take
	const references to diff_sptrs instead of copying.
	* src/abg-dwarf-reader.cc (die_location): Take a const
	reference to translation_unit_sptr instead of copying.
	* src/abg-ir.cc (function_decls_alias): Take const references
	to elf_symbol_sptrs instead of copying.
	(virtual_member_function_less_than::operator()): Take const
	reference to location instead of copying.
	(methods_equal_modulo_elf_symbol): Modify objects through
	given shared pointers rather than copies thereof.
	* src/abg-suppression.cc
	(function_suppression::suppresses_function): Take const
	references to elf_symbol_sptrs instead of copying them.
	* tools/abipkgdiff.cc (create_private_types_suppressions):
	Take const reference to package_sptr instead of copying.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 include/abg-diff-utils.h    |  2 +-
 src/abg-comparison-priv.h   |  3 ++-
 src/abg-default-reporter.cc |  4 ++--
 src/abg-dwarf-reader.cc     |  2 +-
 src/abg-ir.cc               | 11 ++++++-----
 src/abg-suppression.cc      |  8 ++++----
 tools/abipkgdiff.cc         |  2 +-
 7 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/include/abg-diff-utils.h b/include/abg-diff-utils.h
index b59ffa04f..d113e8380 100644
--- a/include/abg-diff-utils.h
+++ b/include/abg-diff-utils.h
@@ -1623,7 +1623,7 @@ compute_diff(RandomAccessOutputIterator a_base,
 
       if (snak.has_vertical_edge())
 	{
-	  point p = snak.intermediate();
+	  const point& p = snak.intermediate();
 	  insertion ins(p.x());
 	  ins.inserted_indexes().push_back(p.y());
 	  ses.insertions().push_back(ins);
diff --git a/src/abg-comparison-priv.h b/src/abg-comparison-priv.h
index b82b5d070..8e17155fd 100644
--- a/src/abg-comparison-priv.h
+++ b/src/abg-comparison-priv.h
@@ -981,7 +981,8 @@ struct elf_symbol_comp
   bool
   operator()(const elf_symbol& l, const elf_symbol& r)
   {
-    string name1 = l.get_id_string(), name2 = r.get_id_string();
+    const string& name1 = l.get_id_string();
+    const string& name2 = r.get_id_string();
     return name1 < name2;
   }
 
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index f8572f25c..03c1137fd 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -489,7 +489,7 @@ default_reporter::report(const reference_diff& d, ostream& out,
     report_local_reference_type_changes(d, out, indent);
 
   if (k & SUBTYPE_CHANGE_KIND)
-    if (diff_sptr dif = d.underlying_type_diff())
+    if (const diff_sptr& dif = d.underlying_type_diff())
       {
 	RETURN_IF_BEING_REPORTED_OR_WAS_REPORTED_EARLIER2(dif,
 							  "referenced type");
@@ -680,7 +680,7 @@ default_reporter::report(const array_diff& d, ostream& out,
 						    d.second_array(),
 						    "array type");
 
-  diff_sptr dif = d.element_type_diff();
+  const diff_sptr& dif = d.element_type_diff();
   if (dif->to_be_reported())
     {
       string fn = ir::get_pretty_representation(is_type(dif->first_subject()));
diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 7c56bd8c4..88d775b99 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -8426,7 +8426,7 @@ die_location(const read_context& ctxt, const Dwarf_Die* die)
 
   if (!file.empty() && line != 0)
     {
-      translation_unit_sptr tu = ctxt.cur_transl_unit();
+      const translation_unit_sptr& tu = ctxt.cur_transl_unit();
       location l = tu->get_loc_mgr().create_new_location(file, line, 1);
       return l;
     }
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 358a7a26a..68afd6ea1 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -17746,7 +17746,8 @@ function_decl::get_id() const
 bool
 function_decls_alias(const function_decl& f1, const function_decl& f2)
 {
-  elf_symbol_sptr s1 = f1.get_symbol(), s2 = f2.get_symbol();
+  const elf_symbol_sptr& s1 = f1.get_symbol();
+  const elf_symbol_sptr& s2 = f2.get_symbol();
 
   if (!s1 || !s2)
     return false;
@@ -20490,7 +20491,8 @@ struct virtual_member_function_less_than
 	  {
 	    string fn_filepath, sn_filepath;
 	    unsigned line = 0, column = 0;
-	    location fn_loc = f.get_location(), sn_loc = s.get_location();
+	    const location& fn_loc = f.get_location();
+	    const location& sn_loc = s.get_location();
 	    if (fn_loc)
 	      fn_loc.expand(fn_filepath, line, column);
 	    if (sn_loc)
@@ -20710,10 +20712,9 @@ class_decl::get_hash() const
 /// @return true iff @p f equals @p s without taking their linkage
 /// name or symbol into account.
 static bool
-methods_equal_modulo_elf_symbol(const method_decl_sptr& f,
-				const method_decl_sptr& s)
+methods_equal_modulo_elf_symbol(const method_decl_sptr& first,
+				const method_decl_sptr& second)
 {
-  method_decl_sptr first = f, second = s;
   elf_symbol_sptr saved_first_elf_symbol =
     first->get_symbol();
   elf_symbol_sptr saved_second_elf_symbol =
diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index ae7cc95ce..d6ee9dc56 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -2500,7 +2500,7 @@ function_suppression::suppresses_function(const function_decl* fn,
 	  // function only if the suppression condition matches the
 	  // names of all aliases.
 	  string symbol_name;
-	  elf_symbol_sptr sym = fn->get_symbol();
+	  const elf_symbol_sptr& sym = fn->get_symbol();
 	  ABG_ASSERT(sym);
 	  symbol_name = sym->get_name();
 	  if (sym->has_aliases() && sym->get_alias_from_name(fname))
@@ -2534,7 +2534,7 @@ function_suppression::suppresses_function(const function_decl* fn,
 	  // function only if the suppression condition matches *all*
 	  // the aliases.
 	  string symbol_name;
-	  elf_symbol_sptr sym = fn->get_symbol();
+	  const elf_symbol_sptr& sym = fn->get_symbol();
 	  ABG_ASSERT(sym);
 	  symbol_name = sym->get_name();
 	  if (sym->has_aliases())
@@ -2565,7 +2565,7 @@ function_suppression::suppresses_function(const function_decl* fn,
 	  // function only if the suppression condition matches *all*
 	  // the aliases.
 	  string symbol_name;
-	  elf_symbol_sptr sym = fn->get_symbol();
+	  const elf_symbol_sptr& sym = fn->get_symbol();
 	  ABG_ASSERT(sym);
 	  symbol_name = sym->get_name();
 	  if (sym->has_aliases())
@@ -2604,7 +2604,7 @@ function_suppression::suppresses_function(const function_decl* fn,
   // Check if the "symbol_name", "symbol_name_regexp", and
   // "symbol_name_not_regexp" properties match.
   string fn_sym_name, fn_sym_version;
-  elf_symbol_sptr sym = fn->get_symbol();
+  const elf_symbol_sptr& sym = fn->get_symbol();
   if (sym)
     {
       fn_sym_name = sym->get_name();
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index 445e24068..c24868afd 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -1489,7 +1489,7 @@ create_private_types_suppressions(const package& pkg, const options &opts)
 {
   suppressions_type supprs;
 
-  package_sptr devel_pkg = pkg.devel_package();
+  const package_sptr& devel_pkg = pkg.devel_package();
   if (!devel_pkg
       || !file_exists(devel_pkg->extracted_dir_path())
       || !is_dir(devel_pkg->extracted_dir_path()))
-- 
2.28.0.402.g5ffc5be6b7-goog