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

[Tobias Grosser]

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?

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.

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