[cfe-dev] Parser Stmt/Expr Owning Pointer

[Chris Lattner]

On Dec 8, 2008, at 3:42 PM, Sebastian Redl wrote:

> Chris Lattner wrote:
>>
>> On Dec 7, 2008, at 8:29 AM, Sebastian Redl wrote:
>>
>>> The move emulation of this pointer is specialized to support  
>>> backwards compatibility. The ASTMove class, core of the move  
>>> emulation, cannot only be implicitly converted to a new ASTOwner  
>>> (which will take ownership), but also to an ActionResult or a void  
>>> pointer, which also have ownership. This allows the use of move()  
>>> in many contexts, such as returning an ASTOwner where an  
>>> ActionResult is expected, or passing a void* to the Action by  
>>> calling move(). This is a significant advantage in making the use  
>>> of the smart pointer intuitive: move() is used whenever you give  
>>> up ownership, no matter what receives it.
>>
>> In terms of API, can we have a better name than .move() ?  From the  
>> API perspective, seeing:
>> +      return Idx.move();
>> is really strange.
>>
>> How about .take(), as in "take ownership"?
> I prefer move() over take() for two reasons:
> 1) It is more consistent with the name std::move(), which is what  
> moving will inevitably be called in C++0x.
> 2) take() is loaded with the semantics of llvm::OwningPtr; although  
> the function in question has equivalent semantics in general, there  
> are some subtle differences.

The similarity with owningptr was why I liked it :), what subtle  
differences do you see?

> I personally don't find move() strange at all, but then I've been  
> preoccupied with move semantics and their emulation since I learned  
> of the concept two years ago. (Note: with proper C++0x moving,  
> there'll be no need to call move() anymore. However, due to the  
> reference binding rules of C++03, the only way to achieve this is to  
> allow the highly unsafe practice of moving from const references.)

Yep yep, rvalue refs are very nice.  Unfortunately we probably can't  
actually *use* them in llvm/clang until they are widely deployed in  
vendor compilers.  :(  It sucks to have to write portable code.

What do other people think about this name?

>> Is there any good way to make the type checker catch cases where  
>> ownership is not transfered?  Using "return Idx;" accidentally  
>> would be a very subtle mistake would would end up with a dangling  
>> pointer (if I understand).
> You'd just get a compile error. ASTOwner is neither copyable nor  
> implicitly convertible to a pointer. You move() from it, or get()  
> its pointer, but otherwise there's no way to get the pointer out.

Aha!  That is very very cool.  I really like that, ok :)

>>  Maybe if the *default* was to take ownership it would lead to  
>> accidental null-dereference bugs or instead of dangling pointer bugs?
> There is no default. But there aren't bugs either. Just a slightly  
> annoying requirement to type .move() a lot. You cannot really go  
> wrong, unless you explicitly move from a pointer and try to use it  
> afterwards.

Gotcha, excellent!

>>>
>>> I'd also like to see if the use of the smart pointer, which is a  
>>> word larger than ActionResult and two words larger than a raw  
>>> pointer, negatively affects performance. Do you people have a  
>>> hint, or perhaps even a complete framework, for tracking parser  
>>> performance?
>>
>> Are you on a mac?  If so, "time clang INPUTS/Cocoa.m" with a  
>> release-asserts build is the benchmark of choice right now :)
> Nope, no Mac anywhere in the vicinity (i.e. in my collection, or  
> belonging to anyone I know).

Ok, just give a head's up when you commit and someone else can test  
before/after your change.  If C++ support were farther along, you  
could just #include <iostream> or something, which is a somewhat  
decent metric.

> On a side note, I've written the smart pointers that I ultimately  
> want to use, which are going to be used in the parser and action  
> interfaces. I'll get to change a lot of code again, but I don't  
> think that's a big problem. Anyway, the conversion there will have  
> to be done more gradually, because I can't change Sema and Parser in  
> one big block like I've done the internal parser stuff so far.

I'm all for doing incremental changes where possible, thanks for  
splitting this part out.

+#include "AstGuard.h"

Is "AstGuard.h" going to be renamed at some point?


Overall, I really like the patch.  This is great work, please feel  
free to commit it and if others think "move" should be renamed, it can  
be done as a follow-on.

-Chris