<div dir="ltr">Hi David,<div><br></div><div>Partly this may be to do with my unfamiliarity with the unique_ptr idiom, and may be using it incorrectly. We're talking about pointers to "class Chain", which reside in two containers:</div>
<div><br></div><div><font face="courier new, monospace">std::set<Chain*> AllChains;</font></div><div><font face="courier new, monospace">std::map<unsigned, Chain*> ActiveChains;</font></div><div><br></div><div>
AllChains obviously contains all the chains, and ActiveChains may contain a subset of AllChains (as values); there may be duplicates too. To make this work, I'd need to change AllChains to be a std::set<std::unique_ptr<Chain>>, and would have to use "::get()" to get the unmanaged version of the pointer before it could be passed into ActiveChains, as I'm effectively duplicating the pointer. This seems to hack around the intended semantics (shared_ptr might be better here?).</div>
<div><br></div><div>Secondly the loops become more clunky. From:</div><div><br></div><div><div><font face="courier new, monospace">  for (auto *I : AllChains)</font></div><div><font face="courier new, monospace">    EC.insert(I);</font></div>
<div><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace">  for (auto *I : AllChains) {</font></div><div><font face="courier new, monospace">    for (auto *J : AllChains) {</font></div>
<div><font face="courier new, monospace">      if (I != J && I->rangeOverlapsWith(J))</font></div><div><font face="courier new, monospace">        EC.unionSets(I, J);</font></div><div><font face="courier new, monospace">    }</font></div>
<div><font face="courier new, monospace">  }</font></div></div><div><br></div><div><br></div><div>to:</div><div><br></div><div><div><font face="courier new, monospace">  for (auto &I : AllChains)</font></div><div><font face="courier new, monospace">    EC.insert(I.get());</font></div>
<div><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace">  for (auto &I : AllChains) {</font></div><div><font face="courier new, monospace">    for (auto &J : AllChains) {</font></div>
<div><font face="courier new, monospace">      if (&*I != &*J && I->rangeOverlapsWith(&*J))</font></div><div><font face="courier new, monospace">        EC.unionSets(&*I, &*J);</font></div><div>
<font face="courier new, monospace">    }</font></div><div><font face="courier new, monospace">  }</font></div></div><div><br></div><div>(where I've written &* I could have used .get()).</div><div><br></div><div>Am I misusing the idiom? What would you recommend?</div>
<div><br></div><div>Cheers,</div><div><br></div><div>James</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 8 August 2014 16:27, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Fri, Aug 8, 2014 at 5:43 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk">james@jamesmolloy.co.uk</a>> wrote:<br>

> Thanks Tim,<br>
><br>
> Using unique_ptr ended up making the code harder to read, so I simply added<br>
> a deletion loop which I think is cleaner.<br>
<br>
</div>I haven't looked at this patch in detail, but is there a reasonable<br>
summary of why unique_ptr made the code harder to read?<br>
<br>
I'm really interested in making/encouraging ownership to be more<br>
explicit/clear and would sometimes even encourage it when it hurts<br>
(superficial (which might not be the case here)) readability. It's<br>
sometimes the case that ownership looks awkward when you make it<br>
explicit because it's an awkward ownership scheme and making it<br>
implicit just hinds that complexity (in the not good way of hiding -<br>
making it a trap/difficult for others to correctly modify the code in<br>
the future).<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> Committed in r215199, thankyou to all reviewers!<br>
><br>
> Cheers,<br>
><br>
> James<br>
><br>
><br>
> On 8 August 2014 10:42, Tim Northover <<a href="mailto:t.p.northover@gmail.com">t.p.northover@gmail.com</a>> wrote:<br>
>><br>
>> Just added llvm-commits.<br>
>><br>
>> ================<br>
>> Comment at: lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp:318<br>
>> @@ +317,3 @@<br>
>> +  // "link" register between each inst in the chain.<br>
>> +  std::map<unsigned, Chain*> ActiveChains;<br>
>> +  std::set<Chain*> AllChains;<br>
>> ----------------<br>
>> Tim Northover wrote:<br>
>> > Aren't these chains leaked? It *looks* like you could make this (or the<br>
>> > set) a unique_ptr<Chain>.<br>
>> Also, this should definitely be the set. I hadn't noticed things get<br>
>> removed from ActiveChains when I wrote this. Should have been obvious, in<br>
>> retrospect.<br>
>><br>
>> <a href="http://reviews.llvm.org/D4791" target="_blank">http://reviews.llvm.org/D4791</a><br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div></blockquote></div><br></div>