[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:248
@@ -246,2 +247,3 @@
 
-  // FIXME: Implement copy operations if/when they're needed.
+  StringMap(const StringMap &RHS) :
+    StringMapImpl(static_cast<unsigned>(sizeof(MapEntryTy))),
----------------
hfinkel wrote:
> joker.eph wrote:
> > May be explicit?
> I don't think that I can -- I need the copy-initialization support for:
> 
>   CustomNameFuncs = TLI.CustomNameFuncs;
> 
> in D18513.
The issue is only because the copy assignment operator is defined as:
```
StringMap &operator=(StringMap RHS) {
    StringMapImpl::swap(RHS);
    std::swap(Allocator, RHS.Allocator);
    return *this;
  }
```

I think we could change it to:

```
StringMap &operator=(const StringMap &RHS) {
    StringMap Tmp(RHS);
    StringMapImpl::swap(Tmp);
    std::swap(Allocator, Tmp.Allocator);
    return *this;
  }
StringMap &operator=(const StringMap &&RHS) {
    std::swap(Allocator, std::move(RHS.Allocator));
    StringMapImpl::swap(std::move(RHS));
    return *this;
  }
```

Now the only thing that it buys us is a protection against unexpected/unintended copy construction for a StringMap, is it compelling enough?



http://reviews.llvm.org/D18506