[4/6] C++ify badness_vector, fix leaks

Message ID 20181015151115.6356-5-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Oct. 15, 2018, 3:11 p.m. UTC
  badness_vector is currenlty an open coded vector.  This reimplements
it as a std::vector.

This fixes a few leaks as well:

 - find_oload_champ is leaking every badness vector calculated bar the
   one returned.

 - bv->rank is always leaked, since callers of rank_function only
   xfree the badness_vector pointer, not bv->rank.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdbtypes.c (compare_badness): Change type of parameters to const
	reference.  Adjust to badness_vector being a std::vector now.
	(rank_function): Adjust to badness_vector being a std::vector now.
	* gdbtypes.h (badness_vector): Now a typedef to std::vector.
	(LENGTH_MATCH): Delete.
	(compare_badness): Change type of parameters to const reference.
	(rank_function): Return a badness_vector by value now.
	(find_overload_match): Adjust to badness_vector being a
	std::vector now.  Remove cleanups.
	(find_oload_champ_namespace): 'oload_champ_bv' parameter now a
	badness_vector pointer.
	(find_oload_champ_namespace_loop): 'oload_champ_bv' parameter now
	a badness_vector pointer.  Adjust to badness_vector being a
	std::vector now.  Remove cleanups.
	(find_oload_champ): 'oload_champ_bv' parameter now
	a badness_vector pointer.  Adjust to badness_vector being a
	std::vector now.  Remove cleanups.
---
 gdb/gdbtypes.c | 43 +++++++++++++++++------------------
 gdb/gdbtypes.h | 17 +++++---------
 gdb/valops.c   | 71 +++++++++++++++++++++-------------------------------------
 3 files changed, 53 insertions(+), 78 deletions(-)
  

Comments

Simon Marchi Oct. 17, 2018, 5:44 p.m. UTC | #1
LGTM, just a tiny comment:

On 2018-10-15 11:11 a.m., Pedro Alves wrote:
> @@ -2848,7 +2842,7 @@ find_overload_match (gdb::array_view<value *> args,

>     contained in QUALIFIED_NAME until it either finds a good match or

>     runs out of namespaces.  It stores the overloaded functions in

>     *OLOAD_SYMS, and the badness vector in *OLOAD_CHAMP_BV.  The

> -   calling function is responsible for freeing *OLOAD_SYMS and

> +   calling function is responsible for clearing *OLOAD_SYMS and

>     *OLOAD_CHAMP_BV.  If NO_ADL, argument dependent lookup is not 

>     performned.  */


The previous comment meant that the caller was responsible to free the data
after this function is called.  From what I understand, the new comment means
the caller is responsible to pass empty vectors as inputs, is that right?  If
so, it would be clearer to say "the calling function is responsible for passing
empty vectors for *OLOAD_SYMS and *OLOAD_CHAMP_BV".

I know the opposite wouldn't really make sense (asking the caller to clear the
vector after the call), but like this it sounds ambiguous.

Simon
  

Patch

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 4160d996de..39b7a99017 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3426,21 +3426,21 @@  compare_ranks (struct rank a, struct rank b)
    3 => A is worse than B  */
 
 int
-compare_badness (struct badness_vector *a, struct badness_vector *b)
+compare_badness (const badness_vector &a, const badness_vector &b)
 {
   int i;
   int tmp;
   short found_pos = 0;		/* any positives in c? */
   short found_neg = 0;		/* any negatives in c? */
 
-  /* differing lengths => incomparable */
-  if (a->length != b->length)
+  /* differing sizes => incomparable */
+  if (a.size () != b.size ())
     return 1;
 
   /* Subtract b from a */
-  for (i = 0; i < a->length; i++)
+  for (i = 0; i < a.size (); i++)
     {
-      tmp = compare_ranks (b->rank[i], a->rank[i]);
+      tmp = compare_ranks (b[i], a[i]);
       if (tmp > 0)
 	found_pos = 1;
       else if (tmp < 0)
@@ -3465,19 +3465,16 @@  compare_badness (struct badness_vector *a, struct badness_vector *b)
 }
 
 /* Rank a function by comparing its parameter types (PARMS), to the
-   types of an argument list (ARGS).  Return a pointer to a badness
-   vector.  This has ARGS.size() + 1 entries.  */
+   types of an argument list (ARGS).  Return the badness vector.  This
+   has ARGS.size() + 1 entries.  */
 
-struct badness_vector *
+badness_vector
 rank_function (gdb::array_view<type *> parms,
 	       gdb::array_view<value *> args)
 {
-  int i;
-  struct badness_vector *bv = XNEW (struct badness_vector);
-  size_t min_len = std::min (parms.size (), args.size ());
-
-  bv->length = args.size () + 1;	/* add 1 for the length-match rank.  */
-  bv->rank = XNEWVEC (struct rank, args.size () + 1);
+  /* add 1 for the length-match rank.  */
+  badness_vector bv;
+  bv.reserve (1 + args.size ());
 
   /* First compare the lengths of the supplied lists.
      If there is a mismatch, set it to a high value.  */
@@ -3486,18 +3483,20 @@  rank_function (gdb::array_view<type *> parms,
      arguments and ellipsis parameter lists, we should consider those
      and rank the length-match more finely.  */
 
-  LENGTH_MATCH (bv) = (args.size () != parms.size ())
-		      ? LENGTH_MISMATCH_BADNESS
-		      : EXACT_MATCH_BADNESS;
+  bv.push_back ((args.size () != parms.size ())
+		? LENGTH_MISMATCH_BADNESS
+		: EXACT_MATCH_BADNESS);
 
   /* Now rank all the parameters of the candidate function.  */
-  for (i = 1; i <= min_len; i++)
-    bv->rank[i] = rank_one_type (parms[i - 1], value_type (args[i - 1]),
-				 args[i - 1]);
+  size_t min_len = std::min (parms.size (), args.size ());
+
+  for (size_t i = 0; i < min_len; i++)
+    bv.push_back (rank_one_type (parms[i], value_type (args[i]),
+				 args[i]));
 
   /* If more arguments than parameters, add dummy entries.  */
-  for (i = min_len + 1; i <= args.size (); i++)
-    bv->rank[i] = TOO_FEW_PARAMS_BADNESS;
+  for (size_t i = min_len; i < args.size (); i++)
+    bv.push_back (TOO_FEW_PARAMS_BADNESS);
 
   return bv;
 }
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 731b18d082..f0adec7a20 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1095,13 +1095,9 @@  struct rank
     short subrank;
   };
 
-/* * Struct used for ranking a function for overload resolution.  */
+/* * Used for ranking a function for overload resolution.  */
 
-struct badness_vector
-  {
-    int length;
-    struct rank *rank;
-  };
+typedef std::vector<rank> badness_vector;
 
 /* * GNAT Ada-specific information for various Ada types.  */
 
@@ -1983,8 +1979,6 @@  extern int is_unique_ancestor (struct type *, struct value *);
 
 /* Overload resolution */
 
-#define LENGTH_MATCH(bv) ((bv)->rank[0])
-
 /* * Badness if parameter list length doesn't match arg list length.  */
 extern const struct rank LENGTH_MISMATCH_BADNESS;
 
@@ -2043,10 +2037,11 @@  extern const struct rank NS_INTEGER_POINTER_CONVERSION_BADNESS;
 extern struct rank sum_ranks (struct rank a, struct rank b);
 extern int compare_ranks (struct rank a, struct rank b);
 
-extern int compare_badness (struct badness_vector *, struct badness_vector *);
+extern int compare_badness (const badness_vector &,
+			    const badness_vector &);
 
-extern struct badness_vector *rank_function (gdb::array_view<type *> parms,
-					     gdb::array_view<value *> args);
+extern badness_vector rank_function (gdb::array_view<type *> parms,
+				     gdb::array_view<value *> args);
 
 extern struct rank rank_one_type (struct type *, struct type *,
 				  struct value *);
diff --git a/gdb/valops.c b/gdb/valops.c
index d21980e726..1cf3fd8fd5 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -57,27 +57,26 @@  static struct value *search_struct_method (const char *, struct value **,
 static int find_oload_champ_namespace (gdb::array_view<value *> args,
 				       const char *, const char *,
 				       std::vector<symbol *> *oload_syms,
-				       struct badness_vector **,
+				       badness_vector *,
 				       const int no_adl);
 
 static int find_oload_champ_namespace_loop (gdb::array_view<value *> args,
 					    const char *, const char *,
 					    int, std::vector<symbol *> *oload_syms,
-					    struct badness_vector **, int *,
+					    badness_vector *, int *,
 					    const int no_adl);
 
 static int find_oload_champ (gdb::array_view<value *> args, int,
 			     struct fn_field *,
 			     const std::vector<xmethod_worker_up> *,
-			     struct symbol **, struct badness_vector **);
+			     struct symbol **, badness_vector *);
 
 static int oload_method_static_p (struct fn_field *, int);
 
 enum oload_classification { STANDARD, NON_STANDARD, INCOMPATIBLE };
 
-static enum
-oload_classification classify_oload_match (struct badness_vector *,
-					   int, int);
+static enum oload_classification classify_oload_match
+  (const badness_vector &, int, int);
 
 static struct value *value_struct_elt_for_reference (struct type *,
 						     int, struct type *,
@@ -2508,10 +2507,10 @@  find_overload_match (gdb::array_view<value *> args,
   int ext_method_oload_champ = -1;
 
   /* The measure for the current best match.  */
-  struct badness_vector *method_badness = NULL;
-  struct badness_vector *func_badness = NULL;
-  struct badness_vector *ext_method_badness = NULL;
-  struct badness_vector *src_method_badness = NULL;
+  badness_vector method_badness;
+  badness_vector func_badness;
+  badness_vector ext_method_badness;
+  badness_vector src_method_badness;
 
   struct value *temp = obj;
   /* For methods, the list of overloaded methods.  */
@@ -2584,8 +2583,6 @@  find_overload_match (gdb::array_view<value *> args,
 	  src_method_match_quality = classify_oload_match
 	    (src_method_badness, args.size (),
 	     oload_method_static_p (fns_ptr, src_method_oload_champ));
-
-	  make_cleanup (xfree, src_method_badness);
 	}
 
       if (!xm_worker_vec.empty ())
@@ -2594,7 +2591,6 @@  find_overload_match (gdb::array_view<value *> args,
 						     NULL, &ext_method_badness);
 	  ext_method_match_quality = classify_oload_match (ext_method_badness,
 							   args.size (), 0);
-	  make_cleanup (xfree, ext_method_badness);
 	}
 
       if (src_method_oload_champ >= 0 && ext_method_oload_champ >= 0)
@@ -2716,8 +2712,6 @@  find_overload_match (gdb::array_view<value *> args,
       if (func_oload_champ >= 0)
 	func_match_quality = classify_oload_match (func_badness,
 						   args.size (), 0);
-
-      make_cleanup (xfree, func_badness);
     }
 
   /* Did we find a match ?  */
@@ -2848,7 +2842,7 @@  find_overload_match (gdb::array_view<value *> args,
    contained in QUALIFIED_NAME until it either finds a good match or
    runs out of namespaces.  It stores the overloaded functions in
    *OLOAD_SYMS, and the badness vector in *OLOAD_CHAMP_BV.  The
-   calling function is responsible for freeing *OLOAD_SYMS and
+   calling function is responsible for clearing *OLOAD_SYMS and
    *OLOAD_CHAMP_BV.  If NO_ADL, argument dependent lookup is not 
    performned.  */
 
@@ -2857,7 +2851,7 @@  find_oload_champ_namespace (gdb::array_view<value *> args,
 			    const char *func_name,
 			    const char *qualified_name,
 			    std::vector<symbol *> *oload_syms,
-			    struct badness_vector **oload_champ_bv,
+			    badness_vector *oload_champ_bv,
 			    const int no_adl)
 {
   int oload_champ;
@@ -2878,7 +2872,7 @@  find_oload_champ_namespace (gdb::array_view<value *> args,
    if it isn't.  Other arguments are the same as in
    find_oload_champ_namespace
 
-   It is the caller's responsibility to free *OLOAD_SYMS and
+   It is the caller's responsibility to clear *OLOAD_SYMS and
    *OLOAD_CHAMP_BV.  */
 
 static int
@@ -2887,15 +2881,13 @@  find_oload_champ_namespace_loop (gdb::array_view<value *> args,
 				 const char *qualified_name,
 				 int namespace_len,
 				 std::vector<symbol *> *oload_syms,
-				 struct badness_vector **oload_champ_bv,
+				 badness_vector *oload_champ_bv,
 				 int *oload_champ,
 				 const int no_adl)
 {
   int next_namespace_len = namespace_len;
   int searched_deeper = 0;
-  struct cleanup *old_cleanups;
   int new_oload_champ;
-  struct badness_vector *new_oload_champ_bv;
   char *new_namespace;
 
   if (next_namespace_len != 0)
@@ -2906,9 +2898,6 @@  find_oload_champ_namespace_loop (gdb::array_view<value *> args,
   next_namespace_len +=
     cp_find_first_component (qualified_name + next_namespace_len);
 
-  /* Initialize these to values that can safely be xfree'd.  */
-  *oload_champ_bv = NULL;
-
   /* First, see if we have a deeper namespace we can search in.
      If we get a good match there, use it.  */
 
@@ -2934,7 +2923,6 @@  find_oload_champ_namespace_loop (gdb::array_view<value *> args,
      because this overload mechanism only gets called if there's a
      function symbol to start off with.)  */
 
-  old_cleanups = make_cleanup (xfree, *oload_champ_bv);
   new_namespace = (char *) alloca (namespace_len + 1);
   strncpy (new_namespace, qualified_name, namespace_len);
   new_namespace[namespace_len] = '\0';
@@ -2958,6 +2946,7 @@  find_oload_champ_namespace_loop (gdb::array_view<value *> args,
 				    new_oload_syms);
     }
 
+  badness_vector new_oload_champ_bv;
   new_oload_champ = find_oload_champ (args, new_oload_syms.size (),
 				      NULL, NULL, new_oload_syms.data (),
 				      &new_oload_champ_bv);
@@ -2974,22 +2963,18 @@  find_oload_champ_namespace_loop (gdb::array_view<value *> args,
     {
       *oload_syms = std::move (new_oload_syms);
       *oload_champ = new_oload_champ;
-      *oload_champ_bv = new_oload_champ_bv;
-      do_cleanups (old_cleanups);
+      *oload_champ_bv = std::move (new_oload_champ_bv);
       return 1;
     }
   else if (searched_deeper)
     {
-      xfree (new_oload_champ_bv);
-      discard_cleanups (old_cleanups);
       return 0;
     }
   else
     {
       *oload_syms = std::move (new_oload_syms);
       *oload_champ = new_oload_champ;
-      *oload_champ_bv = new_oload_champ_bv;
-      do_cleanups (old_cleanups);
+      *oload_champ_bv = std::move (new_oload_champ_bv);
       return 0;
     }
 }
@@ -3003,20 +2988,18 @@  find_oload_champ_namespace_loop (gdb::array_view<value *> args,
    or OLOAD_SYMS (whichever is non-NULL) is specified in NUM_FNS.
 
    Return the index of the best match; store an indication of the
-   quality of the match in OLOAD_CHAMP_BV.
-
-   It is the caller's responsibility to free *OLOAD_CHAMP_BV.  */
+   quality of the match in OLOAD_CHAMP_BV.  */
 
 static int
 find_oload_champ (gdb::array_view<value *> args,
 		  int num_fns, struct fn_field *fns_ptr,
 		  const std::vector<xmethod_worker_up> *xm_worker_vec,
 		  struct symbol **oload_syms,
-		  struct badness_vector **oload_champ_bv)
+		  badness_vector *oload_champ_bv)
 {
   int ix;
   /* A measure of how good an overloaded instance is.  */
-  struct badness_vector *bv;
+  badness_vector bv;
   /* Index of best overloaded function.  */
   int oload_champ = -1;
   /* Current ambiguity state for overload resolution.  */
@@ -3029,8 +3012,6 @@  find_oload_champ (gdb::array_view<value *> args,
   gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) + (xm_worker_vec != NULL)
 	      == 1);
 
-  *oload_champ_bv = NULL;
-
   int fn_count = xm_worker_vec != NULL ? xm_worker_vec->size () : num_fns;
 
   /* Consider each candidate in turn.  */
@@ -3073,9 +3054,9 @@  find_oload_champ (gdb::array_view<value *> args,
       bv = rank_function (parm_types,
 			  args.slice (static_offset));
 
-      if (!*oload_champ_bv)
+      if (oload_champ_bv->empty ())
 	{
-	  *oload_champ_bv = bv;
+	  *oload_champ_bv = std::move (bv);
 	  oload_champ = 0;
 	}
       else /* See whether current candidate is better or worse than
@@ -3089,7 +3070,7 @@  find_oload_champ (gdb::array_view<value *> args,
 	    oload_ambiguous = 2;
 	    break;
 	  case 2:		/* New champion, record details.  */
-	    *oload_champ_bv = bv;
+	    *oload_champ_bv = std::move (bv);
 	    oload_ambiguous = 0;
 	    oload_champ = ix;
 	    break;
@@ -3116,7 +3097,7 @@  find_oload_champ (gdb::array_view<value *> args,
 	  for (jj = 0; jj < args.size () - static_offset; jj++)
 	    fprintf_filtered (gdb_stderr,
 			      "...Badness @ %d : %d\n", 
-			      jj, bv->rank[jj].rank);
+			      jj, bv[jj].rank);
 	  fprintf_filtered (gdb_stderr, "Overload resolution "
 			    "champion is %d, ambiguous? %d\n", 
 			    oload_champ, oload_ambiguous);
@@ -3141,7 +3122,7 @@  oload_method_static_p (struct fn_field *fns_ptr, int index)
 /* Check how good an overload match OLOAD_CHAMP_BV represents.  */
 
 static enum oload_classification
-classify_oload_match (struct badness_vector *oload_champ_bv,
+classify_oload_match (const badness_vector &oload_champ_bv,
 		      int nargs,
 		      int static_offset)
 {
@@ -3152,12 +3133,12 @@  classify_oload_match (struct badness_vector *oload_champ_bv,
     {
       /* If this conversion is as bad as INCOMPATIBLE_TYPE_BADNESS
          or worse return INCOMPATIBLE.  */
-      if (compare_ranks (oload_champ_bv->rank[ix],
+      if (compare_ranks (oload_champ_bv[ix],
                          INCOMPATIBLE_TYPE_BADNESS) <= 0)
 	return INCOMPATIBLE;	/* Truly mismatched types.  */
       /* Otherwise If this conversion is as bad as
          NS_POINTER_CONVERSION_BADNESS or worse return NON_STANDARD.  */
-      else if (compare_ranks (oload_champ_bv->rank[ix],
+      else if (compare_ranks (oload_champ_bv[ix],
                               NS_POINTER_CONVERSION_BADNESS) <= 0)
 	worst = NON_STANDARD;	/* Non-standard type conversions
 				   needed.  */