[PATCH] D18506: Add a copy constructor to StringMap

[David Blaikie via llvm-commits]

On Tue, Mar 29, 2016 at 6:20 PM, Mehdi AMINI via llvm-commits <
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...


> ```
>
> 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'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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> 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/865265c1/attachment.html>