[polly] r238090 - Use unique_ptr to clarify ownership of ScopStmt

[David Blaikie]

On Tue, May 26, 2015 at 5:32 AM, Tobias Grosser <tobias at grosser.es> wrote:

> On 05/23/2015 12:19 PM, Johannes Doerfert wrote:
>
>> On 05/23, Tobias Grosser wrote:
>>
>>> Author: grosser
>>> Date: Sat May 23 00:14:09 2015
>>> New Revision: 238090
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=238090&view=rev
>>> Log:
>>> Use unique_ptr to clarify ownership of ScopStmt
>>>
>>> -  typedef std::vector<ScopStmt *> StmtSet;
>>> +  typedef std::vector<std::unique_ptr<ScopStmt>> StmtSet;
>>>
>> I'm not sure about this. Why/Where do you need clarification for the
>> ownership?
>>
>
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).


>
> 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:
>
> "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]
>
> 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.
>

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).


>
> 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.
>
> 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).
>
> [1] http://herbsutter.com/elements-of-modern-c-style/
>
>      SmallPtrSet<const Value *, 8> ReductionBaseValues;
>>> -  for (ScopStmt *Stmt : S)
>>> +  for (auto &Stmt : S)
>>>
>> I liked the * in the type better as you knew what you get. Now I can see
>> only a reference which I usually do not suspected to be a capsuled
>> pointer.
>>
>
> 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.
>
> 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.
>
> Best,
> Tobias
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150526/6a9f13f9/attachment.html>