[llvm] r264906 - Add a copy constructor to StringMap

[Justin Bogner via llvm-commits]

Hal Finkel via llvm-commits <llvm-commits at lists.llvm.org> writes:
> Author: hfinkel
> Date: Wed Mar 30 14:54:56 2016
> New Revision: 264906
>
> URL: http://llvm.org/viewvc/llvm-project?rev=264906&view=rev
> Log:
> Add a copy constructor to StringMap
>
> There is code under review that requires StringMap to have a copy constructor,
> and this makes StringMap more consistent with our other containers (like
> DenseMap) that have copy constructors.
>
> Differential Revision: http://reviews.llvm.org/D18506
>
> Modified:
>     llvm/trunk/include/llvm/ADT/StringMap.h
>     llvm/trunk/unittests/ADT/StringMapTest.cpp
>
> Modified: llvm/trunk/include/llvm/ADT/StringMap.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=264906&r1=264905&r2=264906&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
> +++ llvm/trunk/include/llvm/ADT/StringMap.h Wed Mar 30 14:54:56 2016
> @@ -89,7 +89,8 @@ protected:
>    /// table, returning it.  If the key is not in the table, this returns null.
>    StringMapEntryBase *RemoveKey(StringRef Key);
>  
> -private:
> +  /// Allocate the table with the specified number of buckets and otherwise
> +  /// setup the map as empty.
>    void init(unsigned Size);
>  
>  public:
> @@ -244,7 +245,39 @@ public:
>      return *this;
>    }
>  
> -  // FIXME: Implement copy operations if/when they're needed.
> +  StringMap(const StringMap &RHS) :
> +    StringMapImpl(static_cast<unsigned>(sizeof(MapEntryTy))),
> +    Allocator(RHS.Allocator) {
> +    if (RHS.empty())
> +      return;
> +
> +    // Allocate TheTable of the same size as RHS's TheTable, and set the
> +    // sentinel appropriately (and NumBuckets).
> +    init(RHS.NumBuckets);
> +    unsigned *HashTable = (unsigned *)(TheTable + NumBuckets + 1),
> +             *RHSHashTable = (unsigned *)(RHS.TheTable + NumBuckets + 1);
> +
> +    NumItems = RHS.NumItems;
> +    NumTombstones = RHS.NumTombstones;
> +    for (unsigned I = 0, E = NumBuckets; I != E; ++I) {
> +      MapEntryTy *Bucket = ((MapEntryTy**) RHS.TheTable)[I];

UBSan complains here when this is a tombstone:

    include/llvm/ADT/StringMap.h:264:22: runtime error: upcast of misaligned address 0xffffffffffffffff for type 'llvm::StringMapEntry<int>', which requires 4 byte alignment

I guess this is fairly unlikely to do the wrong thing in practice, but
maybe we should check for the tombstone value before doing the cast?

> +      if (!Bucket || Bucket == getTombstoneVal()) {
> +        TheTable[I] = Bucket;
> +        continue;
> +      }
> +
> +      TheTable[I] = MapEntryTy::Create(Bucket->getKey(), Allocator,
> +                                       Bucket->getValue());
> +      HashTable[I] = RHSHashTable[I];
> +    }
> +
> +    // Note that here we've copied everything from the RHS into this object,
> +    // tombstones included. We could, instead, have re-probed for each key to
> +    // instantiate this new object without any tombstone buckets. The
> +    // assumption here is that items are rarely deleted from most StringMaps,
> +    // and so tombstones are rare, so the cost of re-probing for all inputs is
> +    // not worthwhile.
> +  }
>  
>    AllocatorTy &getAllocator() { return Allocator; }
>    const AllocatorTy &getAllocator() const { return Allocator; }
>
> Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=264906&r1=264905&r2=264906&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/StringMapTest.cpp Wed Mar 30 14:54:56 2016
> @@ -157,6 +157,33 @@ TEST_F(StringMapTest, SmallFullMapTest)
>    EXPECT_EQ(5, Map.lookup("funf"));
>  }
>  
> +TEST_F(StringMapTest, CopyCtorTest) {
> +  llvm::StringMap<int> Map;
> +
> +  Map["eins"] = 1;
> +  Map["zwei"] = 2;
> +  Map["drei"] = 3;
> +  Map.erase("drei");
> +  Map.erase("eins");
> +  Map["veir"] = 4;
> +  Map["funf"] = 5;
> +
> +  EXPECT_EQ(3u, Map.size());
> +  EXPECT_EQ(0, Map.lookup("eins"));
> +  EXPECT_EQ(2, Map.lookup("zwei"));
> +  EXPECT_EQ(0, Map.lookup("drei"));
> +  EXPECT_EQ(4, Map.lookup("veir"));
> +  EXPECT_EQ(5, Map.lookup("funf"));
> +
> +  llvm::StringMap<int> Map2(Map);
> +  EXPECT_EQ(3u, Map2.size());
> +  EXPECT_EQ(0, Map2.lookup("eins"));
> +  EXPECT_EQ(2, Map2.lookup("zwei"));
> +  EXPECT_EQ(0, Map2.lookup("drei"));
> +  EXPECT_EQ(4, Map2.lookup("veir"));
> +  EXPECT_EQ(5, Map2.lookup("funf"));
> +}
> +
>  // A more complex iteration test.
>  TEST_F(StringMapTest, IterationTest) {
>    bool visited[100];
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits