Make HashMap's delete key a const expression.

corelib/src/implementation/hash_map_set.dart

Index: corelib/src/implementation/hash_map_set.dart
diff --git a/corelib/src/implementation/hash_map_set.dart b/corelib/src/implementation/hash_map_set.dart
index 1d949069b9b8b4ea6335014ef21214aa0b611697..07719444829f8ff1ee268af5d656071913b84914 100644
--- a/corelib/src/implementation/hash_map_set.dart
+++ b/corelib/src/implementation/hash_map_set.dart
@@ -7,8 +7,8 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
 
   // The [_keys] list contains the keys inserted in the map.
   // The [_keys] list must be a raw list because it
-  // will contain both elements of type K, and the [_deletedKey] of type
-  // Object.
+  // will contain both elements of type K, and the [_DELETED_KEY] of type
+  // [_DeleteKeySentinel].

[Ivan Posva, 2011/11/17 18:57:47]

DeletedKeySentinel

[Siggi Cherem (dart-lang), 2011/11/17 20:00:21]

Done (I expect you meant here to remove the _ from the type name).

[Ivan Posva, 2011/11/17 21:06:51]

You really, really want to keep this type private otherwise you suffer the same
problems as you did with const Object(). What I meant to say is that you want to
rename the type to _DeletedKeySentinel (note the d) because it refers to the
deleted key and not to the delete operation.

[Siggi Cherem (dart-lang), 2011/11/17 21:13:27]

Fixed - I was assuming that because this type is within corelib_impl (and not
corelib), it wasn't accessible elsewhere anyways.

   // The alternative of declaring the [_keys] list as of type Object
   // does not work, because the HashSetIterator constructor would fail:
   //  HashSetIterator(HashSet<E> set)
@@ -36,19 +36,13 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
   // The current number of deleted entries in the map.
   int _numberOfDeleted;
 
-  // The sentinel when a key is deleted from the map. We cannot use static
-  // const here because we would need to allocate a "const Object()" which
-  // would end up canonicalized and then we cannot distinguish the deleted
-  // key from the canonicalized Object().
-  static Object _deletedKey;
+  // The sentinel when a key is deleted from the map.
+  static final Object _DELETED_KEY = const _DeleteKeySentinel();

[Siggi Cherem (dart-lang), 2011/11/17 18:40:32]

Is there an issue in using an instance that is not [Object], but
[_DeleteKeySentinel] here?

[Ivan Posva, 2011/11/17 18:57:47]

Why is there even a type on this value? I don't think it needs to be there and
if you want to specify a type then you should use _DeletedKeySentinel.

[Siggi Cherem (dart-lang), 2011/11/17 20:00:21]

Done.

 
   // The initial capacity of a hash map.
   static final int _INITIAL_CAPACITY = 8;  // must be power of 2
 
   HashMapImplementation() {
-    if (_deletedKey === null) {
-      _deletedKey = new Object();
-    }
     _numberOfEntries = 0;
     _numberOfDeleted = 0;
     _loadLimit = _computeLoadLimit(_INITIAL_CAPACITY);
@@ -93,7 +87,7 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
       } else if (existingKey == key) {
         // The key is already in the map. Return its slot.
         return hash;
-      } else if ((insertionIndex < 0) && (_deletedKey === existingKey)) {
+      } else if ((insertionIndex < 0) && (_DELETED_KEY === existingKey)) {
         // The slot contains a deleted element. Because previous calls to this
         // method may not have had this slot deleted, we must continue iterate
         // to find if there is a slot with the given key.
@@ -160,7 +154,7 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
     for (int i = 0; i < capacity; i++) {
       Object key = oldKeys[i];

[Ivan Posva, 2011/11/17 18:57:47]

ditto.

[Siggi Cherem (dart-lang), 2011/11/17 20:00:21]

Done.

       // If there is no key, we don't need to deal with the current slot.
-      if (key === null || key === _deletedKey) {
+      if (key === null || key === _DELETED_KEY) {
         continue;
       }
       V value = oldValues[i];
@@ -185,7 +179,7 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
   void operator []=(K key, V value) {
     _ensureCapacity();
     int index = _probeForAdding(key);
-    if ((_keys[index] === null) || (_keys[index] === _deletedKey)) {
+    if ((_keys[index] === null) || (_keys[index] === _DELETED_KEY)) {
       _numberOfEntries++;
     }
     _keys[index] = key;
@@ -214,7 +208,7 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
       V value = _values[index];
       _values[index] = null;
       // Set the key to the sentinel to not break the probing chain.
-      _keys[index] = _deletedKey;
+      _keys[index] = _DELETED_KEY;
       _numberOfDeleted++;
       return value;
     }
@@ -232,7 +226,7 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
   void forEach(void f(K key, V value)) {
     int length = _keys.length;
     for (int i = 0; i < length; i++) {
-      if ((_keys[i] !== null) && (_keys[i] !== _deletedKey)) {
+      if ((_keys[i] !== null) && (_keys[i] !== _DELETED_KEY)) {
         f(_keys[i], _values[i]);
       }
     }
@@ -264,7 +258,7 @@ class HashMapImplementation<K extends Hashable, V> implements HashMap<K, V> {
   bool containsValue(V value) {
     int length = _values.length;
     for (int i = 0; i < length; i++) {
-      if ((_keys[i] !== null) && (_keys[i] !== _deletedKey)) {
+      if ((_keys[i] !== null) && (_keys[i] !== _DELETED_KEY)) {
         if (_values[i] == value) return true;
       }
     }
@@ -388,7 +382,7 @@ class HashSetIterator<E> implements Iterator<E> {
 
   bool hasNext() {
     if (_nextValidIndex >= _entries.length) return false;
-    if (_entries[_nextValidIndex] === HashMapImplementation._deletedKey) {
+    if (_entries[_nextValidIndex] === HashMapImplementation._DELETED_KEY) {
       // This happens in case the set was modified in the meantime.
       // A modification on the set may make this iterator misbehave,
       // but we should never return the sentinel.
@@ -409,7 +403,7 @@ class HashSetIterator<E> implements Iterator<E> {
   void _advance() {
     int length = _entries.length;
     var entry;
-    Object deletedKey = HashMapImplementation._deletedKey;
+    Object deletedKey = HashMapImplementation._DELETED_KEY;

[Ivan Posva, 2011/11/17 18:57:47]

ditto: var or _DeletedKeySentinel

[Siggi Cherem (dart-lang), 2011/11/17 20:00:21]

Done.

     do {
       if (++_nextValidIndex >= length) break;
       entry = _entries[_nextValidIndex];
@@ -424,3 +418,14 @@ class HashSetIterator<E> implements Iterator<E> {
   // iterator will return false.
   int _nextValidIndex;
 }
+
+/**
+ * A singleton sentinel used to represent when a key is deleted from the map.
+ * We can't use [: const Object() :] as a sentinel because it would end up
+ * canonicalized and then we cannot distinguish the deleted key from the
+ * canonicalized Object().
+ */
+class _DeleteKeySentinel implements Hashable {
+  const _DeleteKeySentinel();
+  int hashCode() => 1;

[Ivan Posva, 2011/11/17 18:57:47]

Why do you need the hashCode() method here at all? Object did not have it and
you also should not need to implement Hashable.

[Siggi Cherem (dart-lang), 2011/11/17 20:00:21]

removed. I had some failing tests while implementing static/globals in frog
isolates and this is something I was playing with to help me debug. I didn't
mean to leave it here.

+}