[v2] cleanup: std::weak_ptr use: replace manual lock by std::weak_ptr::lock

Message ID 20200616205911.234578-1-maennich@google.com
State Committed
Headers
Series [v2] cleanup: std::weak_ptr use: replace manual lock by std::weak_ptr::lock |

Commit Message

Matthias Männich June 16, 2020, 8:59 p.m. UTC
  The pattern

	expired() ? shared_ptr<T>() : shared_ptr<T>(*this)

on std::weak_ptr<T> can be simplified by using std::weak_ptr<T>::lock.
Since weak_ptr::lock does this atomically, this patch also addresses
potential data races between the call to expired() and the construction
based on the assumption that the shared_ptr is still around.
Hence, apply this simplification/fix to the code base.

	* src/abg-comparison-priv.h (diff::priv::get_context): improve
	weak_ptr usage.
	(corpus_diff:diff_stats::priv::ctxt): Likewise.
	* src/abg-comparison.cc (corpus_diff::priv::get_context): Likewise.
	(var_diff::type_diff): Likewise.
	* src/abg-ir.cc (elf_symbol::get_next_alias): Likewise.
	(elf_symbol::get_next_common_instance): Likewise.
	(type_base::get_canonical_type): Likewise.
	(qualified_type_def::get_underlying_type): Likewise.
	(pointer_type_def::get_pointed_to_type): Likewise.
	(reference_type_def::get_pointed_to_type): Likewise.
	(array_type_def::subrange_type::get_underlying_type): Likewise.
	(array_type_def::get_element_type): Likewise.
	(typedef_decl::get_underlying_type): Likewise.
	(var_decl::get_type): Likewise.
	(function_type::get_return_type): Likewise.
	(function_decl::get_type): Likewise.
	(function_decl::parameter::get_type): Likewise.
	(class_or_union::get_naming_typedef): Likewise.
	(class_or_union::get_definition_of_declaration): Likewise.
	(class_decl::base_spec::get_base_class): Likewise.
	(template_parameter::get_enclosing_template_decl): Likewise.
	(non_type_tparameter::get_type): Likewise.
	(type_composition::get_composed_type): Likewise.

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-comparison-priv.h |   8 +--
 src/abg-comparison.cc     |  21 +++----
 src/abg-ir.cc             | 114 +++++++-------------------------------
 3 files changed, 30 insertions(+), 113 deletions(-)
  

Comments

Dodji Seketeli June 17, 2020, 8 a.m. UTC | #1
Matthias Maennich <maennich@google.com> a écrit:

> The pattern
>
> 	expired() ? shared_ptr<T>() : shared_ptr<T>(*this)
>
> on std::weak_ptr<T> can be simplified by using std::weak_ptr<T>::lock.
> Since weak_ptr::lock does this atomically, this patch also addresses
> potential data races between the call to expired() and the construction
> based on the assumption that the shared_ptr is still around.
> Hence, apply this simplification/fix to the code base.
>
> 	* src/abg-comparison-priv.h (diff::priv::get_context): improve
> 	weak_ptr usage.
> 	(corpus_diff:diff_stats::priv::ctxt): Likewise.
> 	* src/abg-comparison.cc (corpus_diff::priv::get_context): Likewise.
> 	(var_diff::type_diff): Likewise.
> 	* src/abg-ir.cc (elf_symbol::get_next_alias): Likewise.
> 	(elf_symbol::get_next_common_instance): Likewise.
> 	(type_base::get_canonical_type): Likewise.
> 	(qualified_type_def::get_underlying_type): Likewise.
> 	(pointer_type_def::get_pointed_to_type): Likewise.
> 	(reference_type_def::get_pointed_to_type): Likewise.
> 	(array_type_def::subrange_type::get_underlying_type): Likewise.
> 	(array_type_def::get_element_type): Likewise.
> 	(typedef_decl::get_underlying_type): Likewise.
> 	(var_decl::get_type): Likewise.
> 	(function_type::get_return_type): Likewise.
> 	(function_decl::get_type): Likewise.
> 	(function_decl::parameter::get_type): Likewise.
> 	(class_or_union::get_naming_typedef): Likewise.
> 	(class_or_union::get_definition_of_declaration): Likewise.
> 	(class_decl::base_spec::get_base_class): Likewise.
> 	(template_parameter::get_enclosing_template_decl): Likewise.
> 	(non_type_tparameter::get_type): Likewise.
> 	(type_composition::get_composed_type): Likewise.
>
> Signed-off-by: Matthias Maennich <maennich@google.com>
Applied to master, thanks!

Cheers,
  

Patch

diff --git a/src/abg-comparison-priv.h b/src/abg-comparison-priv.h
index 420e7014421c..6f081b961f1a 100644
--- a/src/abg-comparison-priv.h
+++ b/src/abg-comparison-priv.h
@@ -305,11 +305,7 @@  public:
   /// @returnt a smart pointer to the diff context.
   diff_context_sptr
   get_context() const
-  {
-    if (ctxt_.expired())
-      return diff_context_sptr();
-    return diff_context_sptr(ctxt_);
-  }
+  { return ctxt_.lock(); }
 
   /// Check if a given categorization of a diff node should make it be
   /// filtered out.
@@ -1372,7 +1368,7 @@  struct corpus_diff::diff_stats::priv
 
   diff_context_sptr
   ctxt()
-  {return ctxt_.expired() ? diff_context_sptr() : diff_context_sptr(ctxt_);}
+  { return ctxt_.lock(); }
 }; // end class corpus_diff::diff_stats::priv
 
 void
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 5797c9dd7f3f..81aed36a1f28 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -3355,15 +3355,16 @@  var_diff::second_var() const
 diff_sptr
 var_diff::type_diff() const
 {
-  if (priv_->type_diff_.expired())
+  if (diff_sptr result = priv_->type_diff_.lock())
+    return result;
+  else
     {
-      diff_sptr d = compute_diff(first_var()->get_type(),
-				       second_var()->get_type(),
-				       context());
-      context()->keep_diff_alive(d);
-      priv_->type_diff_ = d;
+      result = compute_diff(first_var()->get_type(), second_var()->get_type(),
+			    context());
+      context()->keep_diff_alive(result);
+      priv_->type_diff_ = result;
+      return result;
     }
-  return diff_sptr(priv_->type_diff_);
 }
 
 /// Return true iff the diff node has a change.
@@ -8901,11 +8902,7 @@  corpus_diff::diff_stats::net_num_leaf_var_changes() const
 /// @return a smart pointer to the context associate with the corpus.
 diff_context_sptr
 corpus_diff::priv::get_context()
-{
-  if (ctxt_.expired())
-    return diff_context_sptr();
-  return diff_context_sptr(ctxt_);
-}
+{ return ctxt_.lock(); }
 
 /// Tests if the lookup tables are empty.
 ///
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 5cc39f59400a..69393028b93d 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1725,11 +1725,7 @@  elf_symbol::is_main_symbol() const
 ///@return the alias, or NULL if there is no alias.
 elf_symbol_sptr
 elf_symbol::get_next_alias() const
-{
-  if (priv_->next_alias_.expired())
-    return elf_symbol_sptr();
-  return elf_symbol_sptr(priv_->next_alias_);
-}
+{ return priv_->next_alias_.lock(); }
 
 
 /// Check if the current elf_symbol has an alias.
@@ -1828,11 +1824,7 @@  elf_symbol::has_other_common_instances() const
 /// @return the next common instance, or nil if there is not any.
 elf_symbol_sptr
 elf_symbol::get_next_common_instance() const
-{
-  if (priv_->next_common_instance_.expired())
-    return elf_symbol_sptr();
-  return elf_symbol_sptr(priv_->next_common_instance_);
-}
+{ return priv_->next_common_instance_.lock(); }
 
 /// Add a common instance to the current common elf symbol.
 ///
@@ -12109,11 +12101,7 @@  type_base::type_base(const environment* e, size_t s, size_t a)
 /// type.
 type_base_sptr
 type_base::get_canonical_type() const
-{
-  if (priv_->canonical_type.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->canonical_type);
-}
+{ return priv_->canonical_type.lock(); }
 
 /// Getter of the canonical type pointer.
 ///
@@ -13400,11 +13388,7 @@  qualified_type_def::get_cv_quals_string_prefix() const
 /// Getter of the underlying type
 shared_ptr<type_base>
 qualified_type_def::get_underlying_type() const
-{
-  if (priv_->underlying_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->underlying_type_);
-}
+{ return priv_->underlying_type_.lock(); }
 
 /// Non-member equality operator for @ref qualified_type_def
 ///
@@ -13639,11 +13623,7 @@  pointer_type_def::operator==(const pointer_type_def& other) const
 /// @return the pointed-to type.
 const type_base_sptr
 pointer_type_def::get_pointed_to_type() const
-{
-  if (priv_->pointed_to_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->pointed_to_type_);
-}
+{ return priv_->pointed_to_type_.lock(); }
 
 /// Getter of a naked pointer to the pointed-to type.
 ///
@@ -13938,11 +13918,7 @@  reference_type_def::operator==(const reference_type_def& o) const
 
 type_base_sptr
 reference_type_def::get_pointed_to_type() const
-{
-  if (pointed_to_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(pointed_to_type_);
-}
+{ return pointed_to_type_.lock(); }
 
 bool
 reference_type_def::is_lvalue() const
@@ -14258,11 +14234,7 @@  array_type_def::subrange_type::subrange_type(const environment* env,
 /// @return the underlying type.
 type_base_sptr
 array_type_def::subrange_type::get_underlying_type() const
-{
-  if (priv_->underlying_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->underlying_type_);
-}
+{ return priv_->underlying_type_.lock(); }
 
 /// Setter of the underlying type of the subrange, that is, the type
 /// that defines the range.
@@ -14796,11 +14768,7 @@  array_type_def::operator==(const type_base& o) const
 /// @return the type of an array element.
 const type_base_sptr
 array_type_def::get_element_type() const
-{
-  if (priv_->element_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->element_type_);
-}
+{ return priv_->element_type_.lock(); }
 
 /// Setter of the type of array element.
 ///
@@ -15666,11 +15634,7 @@  typedef_decl::get_pretty_representation(bool internal,
 /// @return the underlying_type.
 type_base_sptr
 typedef_decl::get_underlying_type() const
-{
-  if (priv_->underlying_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->underlying_type_);
-}
+{ return priv_->underlying_type_.lock(); }
 
 /// This implements the ir_traversable_base::traverse pure virtual
 /// function.
@@ -15760,11 +15724,7 @@  var_decl::var_decl(const string&	name,
 /// @return the type of the variable.
 const type_base_sptr
 var_decl::get_type() const
-{
-  if (priv_->type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->type_);
-}
+{ return priv_->type_.lock(); }
 
 /// Getter of the type of the variable.
 ///
@@ -16357,11 +16317,7 @@  function_type::function_type(const environment* env,
 /// @return the return type.
 type_base_sptr
 function_type::get_return_type() const
-{
-  if (priv_->return_type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->return_type_);
-}
+{ return priv_->return_type_.lock(); }
 
 /// Setter of the return type of the current instance of @ref
 /// function_type.
@@ -17203,11 +17159,7 @@  function_decl::get_first_non_implicit_parm() const
 /// @return the type of the current instance of @ref function_decl.
 const shared_ptr<function_type>
 function_decl::get_type() const
-{
-  if (priv_->type_.expired())
-    return function_type_sptr();
-  return function_type_sptr(priv_->type_);
-}
+{ return priv_->type_.lock(); }
 
 /// Fast getter of the type of the current instance of @ref function_decl.
 ///
@@ -17727,11 +17679,7 @@  function_decl::parameter::parameter(const type_base_sptr	type,
 
 const type_base_sptr
 function_decl::parameter::get_type()const
-{
-  if (priv_->type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->type_);
-}
+{ return priv_->type_.lock(); }
 
 /// @return a copy of the type name of the parameter.
 interned_string
@@ -18567,11 +18515,7 @@  class_or_union::set_is_declaration_only(bool f)
 /// @return the naming typedef, if any.  Otherwise, returns nil.
 typedef_decl_sptr
 class_or_union::get_naming_typedef() const
-{
-  if (priv_->naming_typedef_.expired())
-    return typedef_decl_sptr();
-  return typedef_decl_sptr(priv_->naming_typedef_);
-}
+{ return priv_->naming_typedef_.lock(); }
 
 /// Set the naming typedef of the current instance of @ref class_decl.
 ///
@@ -18609,11 +18553,7 @@  class_or_union::set_definition_of_declaration(class_or_union_sptr d)
 /// @return the definition of this decl-only class.
 const class_or_union_sptr
 class_or_union::get_definition_of_declaration() const
-{
-  if (priv_->definition_of_declaration_.expired())
-    return class_or_union_sptr();
-  return class_or_union_sptr(priv_->definition_of_declaration_);
-}
+{ return priv_->definition_of_declaration_.lock(); }
 
 ///  If this @ref class_or_union is declaration-only, get its
 ///  definition, if any.
@@ -19916,11 +19856,7 @@  class_decl::base_spec::base_spec(const class_decl_sptr& base,
 /// @return the base class.
 class_decl_sptr
 class_decl::base_spec::get_base_class() const
-{
-  if (priv_->base_class_.expired())
-    return class_decl_sptr();
-  return class_decl_sptr(priv_->base_class_);
-}
+{ return priv_->base_class_.lock(); }
 
 /// Getter of the "is-virtual" proprerty of the base class specifier.
 ///
@@ -21896,11 +21832,7 @@  template_parameter::get_index() const
 
 const template_decl_sptr
 template_parameter::get_enclosing_template_decl() const
-{
-  if (priv_->template_decl_.expired())
-    return template_decl_sptr();
-  return template_decl_sptr(priv_->template_decl_);
-}
+{ return priv_->template_decl_.lock(); }
 
 bool
 template_parameter::get_hashing_has_started() const
@@ -22068,11 +22000,7 @@  non_type_tparameter::non_type_tparameter(unsigned		index,
 /// @return the type of the template parameter.
 const type_base_sptr
 non_type_tparameter::get_type() const
-{
-  if (priv_->type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->type_);
-}
+{ return priv_->type_.lock(); }
 
 /// Get the hash value of the current instance.
 ///
@@ -22241,11 +22169,7 @@  type_composition::type_composition(unsigned		index,
 /// @return the composed type.
 const type_base_sptr
 type_composition::get_composed_type() const
-{
-  if (priv_->type_.expired())
-    return type_base_sptr();
-  return type_base_sptr(priv_->type_);
-}
+{ return priv_->type_.lock(); }
 
 /// Setter for the resulting composed type.
 ///