[PATCH] D18506: Add a copy constructor to StringMap

[Hal Finkel via llvm-commits]

hfinkel 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))),
----------------
joker.eph wrote:
> 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?
> 
> Now the only thing that it buys us is a protection against unexpected/unintended copy construction for a StringMap, is it compelling enough?

Personally, I don't see a reason to be so careful about copying a StringMap, more careful, that is, than for other similar data structures. We have non-explicit copy constructors on DenseMap, etc. Everyone needs to be just as careful about copying those. As a user of containers, I tend to find such restrictions annoying. Obviously, people have different opinions on this topic ;)



http://reviews.llvm.org/D18506