<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 29, 2016 at 6:20 PM, Mehdi AMINI via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">joker.eph added inline comments.<br>
<br>
================<br>
</span><span class="">Comment at: include/llvm/ADT/StringMap.h:248<br>
@@ -246,2 +247,3 @@<br>
<br>
</span><span class="">-  // FIXME: Implement copy operations if/when they're needed.<br>
+  StringMap(const StringMap &RHS) :<br>
+    StringMapImpl(static_cast<unsigned>(sizeof(MapEntryTy))),<br>
</span><span class="">----------------<br>
hfinkel wrote:<br>
> joker.eph wrote:<br>
</span>> > May be explicit?<br>
> I don't think that I can -- I need the copy-initialization support for:<br>
><br>
>   CustomNameFuncs = TLI.CustomNameFuncs;<br>
><br>
> in D18513.<br>
The issue is only because the copy assignment operator is defined as:<br>
```<br>
StringMap &operator=(StringMap RHS) {<br>
    StringMapImpl::swap(RHS);<br>
    std::swap(Allocator, RHS.Allocator);<br>
    return *this;<br>
  }<br>
```<br>
<br>
I think we could change it to:<br>
<br>
```<br>
StringMap &operator=(const StringMap &RHS) {<br>
    StringMap Tmp(RHS);<br>
    StringMapImpl::swap(Tmp);<br>
    std::swap(Allocator, Tmp.Allocator);<br>
    return *this;<br>
  }<br>
StringMap &operator=(const StringMap &&RHS) {<br>
    std::swap(Allocator, std::move(RHS.Allocator));<br>
    StringMapImpl::swap(std::move(RHS));<br>
    return *this;<br>
  }<br></blockquote><div><br></div><div>That would be a somewhat scary move op - leaving possibly large allocations in the moved-from object... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
```<br>
<br>
Now the only thing that it buys us is a protection against unexpected/unintended copy construction for a StringMap, is it compelling enough?<br></blockquote><div><br></div><div>Yeah, I'd be pretty uncomfortable making a copy ctor explicit - I'm with Hal here. Make things obvious value types & leave it up to users to choose when to copy or not, etc. (I've seen codebases that try to make copies explicit operations (even named functions) - Google's style guide no longer has any such wording/discouragement & that's for the best, I think)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<a href="http://reviews.llvm.org/D18506" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18506</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>