<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 26, 2015 at 5:32 AM, Tobias Grosser <span dir="ltr"><<a href="mailto:tobias@grosser.es" target="_blank">tobias@grosser.es</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 05/23/2015 12:19 PM, Johannes Doerfert wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 05/23, Tobias Grosser wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: grosser<br>
Date: Sat May 23 00:14:09 2015<br>
New Revision: 238090<br>
<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D238090-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=jkajZ41jzBPFSHjUkHUrlwPEgfNdSLGnTsURbf2ZHQ0&s=YCQJGU20c3RU-anJTNIg0ZsHfxokI0gfpenTxcT_0Ng&e=" target="_blank">http://llvm.org/viewvc/llvm-project?rev=238090&view=rev</a><br>
Log:<br>
Use unique_ptr to clarify ownership of ScopStmt<br>
<br>
-  typedef std::vector<ScopStmt *> StmtSet;<br>
+  typedef std::vector<std::unique_ptr<ScopStmt>> StmtSet;<br>
</blockquote>
I'm not sure about this. Why/Where do you need clarification for the<br>
ownership?<br></blockquote></span></blockquote><div><br>It's pretty easy when handling raw pointers to forget which ones are owning or not - the APIs that produce these owning pointers before they're added to a data structure are particularly error prone (look at the code that populates the StmtSet - a raw pointer owns the value through several conditionals before it's added to the vector - any early-exit path through that code could relatively easily leak memory, so moving to unique_ptr which is implicitly cleaned up even in such an early-exit is a win for maintainability).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote>
<br></span>
This patch was a first mini-step of a general push towards 'no delete' in our source code. The general practice in C++11 code seems to be:<br>
<br>
"Always use the standard smart pointers, and non-owning raw pointers. Never use owning raw pointers and delete, except in rare cases when implementing your own low-level data structure (and even then keep that well encapsulated inside a class boundary)." [1]<br>
<br>
David Blaike has been working since several months on moving LLVM into this direction and I wanted to start this effort on the Polly side as well.<br></blockquote><div><br>Yep - it's not so much a directed goal for me, but an opportunistic one. Whenever we find leak bugs or similar issues, I try to move in a direction that will avoid further leaks - using strict ownership like these smart pointers is a great way to do it (it's not uncommon that, while cleaning up one memory leak by using this technique, we find/fix a few others - and, of course, make it much harder to accidentally introduce more of them later).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
If this is in any sense controversial (please tell me), I am happy to back this out and have a discussion to reconsider this goal.<br>
<br>
My personal motivation are some memory issues I have (in some prototype), which would benefit from more memory safety in general (but most likely not from this specific commit).<br>
<br>
[1] <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__herbsutter.com_elements-2Dof-2Dmodern-2Dc-2Dstyle_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=jkajZ41jzBPFSHjUkHUrlwPEgfNdSLGnTsURbf2ZHQ0&s=X2tTmcNYI-mtHQRcrykXjCGx78LKvHgyZg73Ueok-1Y&e=" target="_blank">http://herbsutter.com/elements-of-modern-c-style/</a><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
    SmallPtrSet<const Value *, 8> ReductionBaseValues;<br>
-  for (ScopStmt *Stmt : S)<br>
+  for (auto &Stmt : S)<br>
</blockquote>
I liked the * in the type better as you knew what you get. Now I can see<br>
only a reference which I usually do not suspected to be a capsuled pointer.<br>
</blockquote>
<br></span>
I agree, the original code was slightly more readable. On the other hand, this iterator clarifies that we indeed only get a reference (and not a separate copy) of the statement. In commit 238090, a similar change helped me to realize that we did an unnecessary copy of a std::pair which could just have been a reference in the range loop.<br>
<br>
I don't feel super strong about this (it worked OK until now), but my general feeling is that using the C++11 managed pointers will be generally beneficial. I am happy to year opinions on this.<br>
<br>
Best,<br>
Tobias<div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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>
</div></div></blockquote></div><br></div></div>