[PATCH] D18506: Add a copy constructor to StringMap

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: include/llvm/ADT/StringMap.h:261
@@ +260,3 @@
+    NumItems = RHS.NumItems;
+    NumTombstones = RHS.NumTombstones;
+    for (unsigned I = 0, E = NumBuckets; I != E; ++I) {
----------------
hfinkel wrote:
> joker.eph wrote:
> > I wonder if we really want to keep the tombstones or if it wouldn't be better to reinsert the elements?
> I don't have a good feeling for this; it seems like this could mean one of two things:
> 
>  1. We start with the same number of buckets as the RHS; in this case keeping the tombstones vs. not just affects whether we write a 0 or -1 into the pointer array for the empty slot (although insertion will prefer that slot over further probing).
>  2. Start with a minimal number of buckets. Potentially more memory efficient, but essentially leads to "rehashing" (which in this case means re-probing for all inputs).
> 
> What do you prefer?
It seems to me that when looking for an entry in the map, the search stops when it find an empty slot, but continue when there is a tombstone. If I understand your 1), it is just replacing tombstones with empty buckets, but that would break the search for existing entries that are after the tombstones.

The 2) is basically what I was thinking of, eventually allocating the same number of buckets, but indeed reinserting each entry (reusing the existing known hash). Not that I have a strong preference, I was rather wondering if you considered this and what is the rational for one or the other (and if there is a reason, I'd rather have it documented in the code for future reference).
I hope David could chime in with an opinion.

================
Comment at: include/llvm/ADT/StringMap.h:272
@@ +271,3 @@
+      HashTable[I] = RHSHashTable[I];
+    }
+  }
----------------
hfinkel wrote:
> joker.eph wrote:
> > The hashes have to be inserted as well I think (after the buckets), I may miss where it happened?
> Yes; that's what the
> 
>   HashTable[I] = RHSHashTable[I];
> 
> does.
Obviously...



http://reviews.llvm.org/D18506