[PATCH] D18506: Add a copy constructor to StringMap

[Mehdi Amini via llvm-commits]

> On Mar 29, 2016, at 10:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Mar 29, 2016 at 6:20 PM, Mehdi AMINI via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> 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;
>   }
> 
> That would be a somewhat scary move op - leaving possibly large allocations in the moved-from object... 

Oh yeah forgot to move-construct a temp to force the destructore to run ASAP...
(and I'd probably implement a swap operator to avoid the code duplication)


>  
> ```
> 
> Now the only thing that it buys us is a protection against unexpected/unintended copy construction for a StringMap, is it compelling enough?
> 
> 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 would agree if there was a way to express intent at call site in C++, it's all implicit. We can agree to disagree philosophically here :)

-- 
Mehdi



> . (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)
>  
> 
> 
> 
> http://reviews.llvm.org/D18506 <http://reviews.llvm.org/D18506>
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160329/9477cf6f/attachment.html>