[RFA,08/12] Remove value::next and value::released

Message ID 20180405211507.6103-9-tom@tromey.com
State New
Headers show
Series
  • (somewhat) clean up struct value ownership
Related show

Commit Message

Tom Tromey April 5, 2018, 9:15 p.m.
This patch converts all_values to simply hold a list of references to
values.  Now, there's no need to have a value record whether or not it
is released -- there is only a single reference-counting mechanism for
values.  So, this also removes value::next, value::released, and
value_next.

ChangeLog
2018-04-05  Tom Tromey  <tom@tromey.com>

	* value.c (struct value) <released, next>: Remove.
	(all_values): Now a std::vector.
	(allocate_value_lazy): Update.
	(value_next): Remove.
	(value_mark, value_free_to_mark, release_value)
	(value_release_to_mark): Update.
---
 gdb/ChangeLog |  9 ++++++
 gdb/value.c   | 95 +++++++++++++++++------------------------------------------
 2 files changed, 36 insertions(+), 68 deletions(-)

-- 
2.13.6

Comments

Pedro Alves April 6, 2018, 7:32 p.m. | #1
On 04/05/2018 10:15 PM, Tom Tromey wrote:
>  value_release_to_mark (const struct value *mark)

>  {


...

> +  std::reverse (result.begin (), result.end ());

>    return result;

>  }


Is there a comment somewhere mentioning the order of the
returned values?

Thanks,
Pedro Alves
Tom Tromey April 6, 2018, 9:40 p.m. | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> On 04/05/2018 10:15 PM, Tom Tromey wrote:
>> value_release_to_mark (const struct value *mark)

>> {


Pedro> ...

>> +  std::reverse (result.begin (), result.end ());

>> return result;

>> }


Pedro> Is there a comment somewhere mentioning the order of the
Pedro> returned values?

Nope.  value.h doesn't generally seem to have comments; but since really
it should, I've done this:

/* Release values from the value chain and return them.  Values
   created after MARK are released.  If MARK is nullptr, or if MARK is
   not found on the value chain, then all values are released.  Values
   are returned in reverse order of creation; that is, newest
   first.  */

extern std::vector<value_ref_ptr> value_release_to_mark
    (const struct value *mark);


Then in value.c I added the usual:

/* See value.h.  */

std::vector<value_ref_ptr>
value_release_to_mark (const struct value *mark)
{
...

Tom

Patch

diff --git a/gdb/value.c b/gdb/value.c
index fed722da51..3137bf48f8 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -198,9 +198,6 @@  struct value
      used instead of read_memory to enable extra caching.  */
   unsigned int stack : 1;
 
-  /* If the value has been released.  */
-  unsigned int released : 1;
-
   /* Location of value (if lval).  */
   union
   {
@@ -309,12 +306,6 @@  struct value
   LONGEST embedded_offset;
   LONGEST pointed_to_offset;
 
-  /* Values are stored in a chain, so that they can be deleted easily
-     over calls to the inferior.  Values assigned to internal
-     variables, put into the value history or exposed to Python are
-     taken off this list.  */
-  struct value *next;
-
   /* Actual contents of the value.  Target byte-order.  NULL or not
      valid if lazy is nonzero.  */
   gdb_byte *contents;
@@ -891,7 +882,7 @@  static std::vector<value_ref_ptr> value_history;
    (except for those released by calls to release_value)
    This is so they can be freed after each command.  */
 
-static struct value *all_values;
+static std::vector<value_ref_ptr> all_values;
 
 /* Allocate a lazy value for type TYPE.  Its actual content is
    "lazily" allocated too: the content field of the return value is
@@ -912,8 +903,6 @@  allocate_value_lazy (struct type *type)
 
   val = XCNEW (struct value);
   val->contents = NULL;
-  val->next = all_values;
-  all_values = val;
   val->type = type;
   val->enclosing_type = type;
   VALUE_LVAL (val) = not_lval;
@@ -929,6 +918,7 @@  allocate_value_lazy (struct type *type)
 
   /* Values start out on the all_values chain.  */
   val->reference_count = 1;
+  all_values.emplace_back (val);
 
   return val;
 }
@@ -1070,12 +1060,6 @@  allocate_optimized_out_value (struct type *type)
 
 /* Accessor methods.  */
 
-struct value *
-value_next (const struct value *value)
-{
-  return value->next;
-}
-
 struct type *
 value_type (const struct value *value)
 {
@@ -1573,7 +1557,9 @@  deprecated_value_modifiable (const struct value *value)
 struct value *
 value_mark (void)
 {
-  return all_values;
+  if (all_values.empty ())
+    return nullptr;
+  return all_values.back ().get ();
 }
 
 /* Take a reference to VAL.  VAL will not be deallocated until all
@@ -1626,16 +1612,11 @@  value_decref (struct value *val)
 void
 value_free_to_mark (const struct value *mark)
 {
-  struct value *val;
-  struct value *next;
-
-  for (val = all_values; val && val != mark; val = next)
-    {
-      next = val->next;
-      val->released = 1;
-      value_decref (val);
-    }
-  all_values = val;
+  auto iter = std::find (all_values.begin (), all_values.end (), mark);
+  if (iter == all_values.end ())
+    all_values.clear ();
+  else
+    all_values.erase (iter + 1, all_values.end ());
 }
 
 /* Remove VAL from the chain all_values
@@ -1645,40 +1626,25 @@  value_ref_ptr
 release_value (struct value *val)
 {
   struct value *v;
-  bool released = false;
 
   if (val == nullptr)
     return value_ref_ptr ();
 
-  if (all_values == val)
-    {
-      all_values = val->next;
-      val->next = NULL;
-      released = true;
-    }
-  else
+  std::vector<value_ref_ptr>::reverse_iterator iter;
+  for (iter = all_values.rbegin (); iter != all_values.rend (); ++iter)
     {
-      for (v = all_values; v; v = v->next)
+      if (*iter == val)
 	{
-	  if (v->next == val)
-	    {
-	      v->next = val->next;
-	      val->next = NULL;
-	      released = true;
-	      break;
-	    }
+	  value_ref_ptr result = *iter;
+	  all_values.erase (iter.base () - 1);
+	  return result;
 	}
     }
 
-  if (!released)
-    {
-      /* We must always return an owned reference.  Normally this
-	 happens because we transfer the reference from the value
-	 chain, but in this case the value was not on the chain.  */
-      value_incref (val);
-    }
-
-  return value_ref_ptr (val);
+  /* We must always return an owned reference.  Normally this happens
+     because we transfer the reference from the value chain, but in
+     this case the value was not on the chain.  */
+  return value_ref_ptr (value_incref (val));
 }
 
 /* Release all values up to mark  */
@@ -1686,23 +1652,16 @@  std::vector<value_ref_ptr>
 value_release_to_mark (const struct value *mark)
 {
   std::vector<value_ref_ptr> result;
-  struct value *next;
 
-  for (next = all_values; next; next = next->next)
+  auto iter = std::find (all_values.begin (), all_values.end (), mark);
+  if (iter == all_values.end ())
+    std::swap (result, all_values);
+  else
     {
-      next->released = 1;
-      result.emplace_back (next);
-
-      if (next->next == mark)
-	{
-	  struct value *save = next->next;
-	  next->next = NULL;
-	  next = save;
-	  break;
-	}
+      std::move (iter + 1, all_values.end (), std::back_inserter (result));
+      all_values.erase (iter + 1, all_values.end ());
     }
-
-  all_values = next;
+  std::reverse (result.begin (), result.end ());
   return result;
 }