Change the stackmap builder API.

runtime/vm/code_descriptors.h

Index: runtime/vm/code_descriptors.h
diff --git a/runtime/vm/code_descriptors.h b/runtime/vm/code_descriptors.h
index 5d2ac8e3915c03abf7a5a5914c0651d0a2ae995b..6a4630fc9cdb99a73067c19fd9289d098100612f 100644
--- a/runtime/vm/code_descriptors.h
+++ b/runtime/vm/code_descriptors.h
@@ -59,42 +59,15 @@ class DescriptorList : public ZoneAllocated {
 };
 
 
-class StackmapBuilder : public ZoneAllocated {
+class StackmapTableBuilder : public ZoneAllocated {
  public:
-  StackmapBuilder() :
-      builder_(new BitmapBuilder()),
+  StackmapTableBuilder() :
       stack_map_(Stackmap::ZoneHandle()),
       list_(GrowableObjectArray::ZoneHandle(
           GrowableObjectArray::New(Heap::kOld))) { }
-  ~StackmapBuilder() { }
+  ~StackmapTableBuilder() { }
 
-  // Gets state of stack slot (object or regular value).
-  bool IsSlotObject(intptr_t stack_slot) const {
-    ASSERT(builder_ != NULL);
-    return builder_->Get(stack_slot);
-  }
-  // Sets stack slot as containing an object.
-  void SetSlotAsObject(intptr_t stack_slot) {
-    ASSERT(builder_ != NULL);
-    builder_->Set(stack_slot, true);
-  }
-  // Sets stack slot as containing regular value.
-  void SetSlotAsValue(intptr_t stack_slot) {
-    ASSERT(builder_ != NULL);
-    builder_->Set(stack_slot, false);
-  }
-  // Sets min..max (inclusive) as stack slots containing objects.
-  void SetSlotRangeAsObject(intptr_t min_stack_slot, intptr_t max_stack_slot) {
-    ASSERT(builder_ != NULL);
-    builder_->SetRange(min_stack_slot, max_stack_slot, true);
-  }
-  // Sets min..max (inclusive) as stack slots containing regular values.
-  void SetSlotRangeAsValue(intptr_t min_stack_slot, intptr_t max_stack_slot) {
-    ASSERT(builder_ != NULL);
-    builder_->SetRange(min_stack_slot, max_stack_slot, false);
-  }
-
-  void AddEntry(intptr_t pc_offset);
+  void AddEntry(intptr_t pc_offset, BitmapBuilder* bitmap);
 

[siva, 2012/07/31 19:03:01]

I would retain a StackmapBuilder class which has a BitmapBuilder field in it and
defines all the methods that you have deleted here:
IsSlotObject,
SetSlotAsObject methods.
...
...
The AddEntry method would take a StackmapBuilder object and not a BitmapBuilder
object.

This would make the stackmap generation code much more readable than the current
form of just setting true, false etc. in the bitmap.

[Kevin Millikin (Google), 2012/08/01 09:34:33]

Thanks for the comment.

I agree with the naming issue (Slava raised it to), but a separate wrapper class
seems too heavy.  There will be exactly one call in the code to SetSlotAsObject,
one to IsSlotObject, and none at all to the other methods:

for (intptr_t i = 0; i < safepoints_.length(); ++i) {
  if (range->CanCover(safepoints_[i].position)) {
    safepoints_[i].stack_bitmap->Set(spill_index, true);
  }
}

(Replace Set(spill_index, true) by SetSlotAsObject(spill_index)).  It seems too
much to wrap the whole bitmap builder, when the interpretation is pretty obvious
in the code above.

I'll hold off on making any change for now, but a lightweight solution for
better naming might be a static function on the StackmapTableBuilder.

   bool Verify();
 
@@ -104,10 +77,9 @@ class StackmapBuilder : public ZoneAllocated {
   intptr_t Length() const { return list_.Length(); }
   RawStackmap* Map(int index) const;
 
-  BitmapBuilder* builder_;
   Stackmap& stack_map_;
   GrowableObjectArray& list_;
-  DISALLOW_COPY_AND_ASSIGN(StackmapBuilder);
+  DISALLOW_COPY_AND_ASSIGN(StackmapTableBuilder);
 };