RAII

[renehiemstra]

I've closed #658 [(https://github.com//pull/658)] and reopened it here in order to get a clean git commit history. This second attempt is implemented from scratch.

I'll summarize the pull request and the revised design:

Objective: The point of the contribution is to add some metamethods to enable RAII in order to implement smart containers and smart pointers, like std::string, std::vector and std::unique_ptr, std::shared_ptr, boost:offset_ptr in C++.

The design does not introduce any breaking changes. No new keywords are introduced. Heap resources are acquired and released using the regular C stdlib functions such malloc and free, leaving memory allocation in the hands of the programmer.

New metamethods: The following metamethods are implemented:

1. __init(self : &A)
2. __dtor(self : &A)
3. __copy(from : &A, to : &B)

If implemented, these methods are inserted judiciously during the type checking phase, implemented in terralib.lua. All these metamethods can be implemented as macro's or as terra functions.

I will explain the metamethods in some more depth:

A managed type is one that implements the above metamethods. In the following I assume struct A is a managed type.

• __init is used to initialize managed variables:

    A.metamethods.__init(self : &A)

The implementation checks for an __init metamethod in any defvar statement: var a : A and applies it if it exists.

• __dtor can be used to free heap memory

    A.metamethods.__dtor(self : &A)

The implementation adds a deferred call to __dtor (right before any return statements) for any variable local to the current scope that is not returned. Hence, it is tied to the lifetime of the object. __dtor is also called before any copy-assignment.

• __copy enables specialized copy-assignment and, combined with __init, copy construction. __copy takes two arguments, which can be different, as long as one of them is a managed type, e.g.

    A.metamethods.__copy(from : &A, to : &B)

and / or

    A.metamethods.__copy(from : &B, to : &A)

__copy can be an overloaded function. In object construction, in case of b below, __copy is combined with __init to perform copy construction:

    var a : A
    a.data = ...
    var b = a

which is the same as writing

    var a : A
    a.data = ...
    var b : A
    b = a

I think the above covers all the use cases of smart containers and pointers in c++. I still need to make the metamethods work properly in the case of compositional API's, like mentioned in #658 (vector(vector(int)) or a vector(string)).

First, let's verify that the CI is passing. Over the next few days I'll add some more tests to test managed data-structures.

[elliottslaughter]

Thanks for submitting this. Looks like CI is still having trouble.

You removed __move in this version?

I think it might help to go through some representative code samples for different patterns and show:

1. What metamethods does the compiler check for? (I guess all of them, all the time? But this wasn't clear.)
2. Assuming those metamethods exist, what calls get inserted where?

It sounds like the compositional aspect is still unimplemented here.

[renehiemstra]

Thanks for considering the pull request.

Regarding the CI. I forgot to uncomment the SDKROOT code for macos (issue #657 ). Also, it seems like the last two months the CI is broken. In particular, the test fakeasm.t segfaults on ubuntu. Maybe, we can give it another try.

I am preparing some code examples (shall I post them here?) In short, the metamethods are checked all the time. The check for metamethods.__dtor is done once in checkblock(...) (which checks a scoped environment) and metamethods.(__init, __copy, __dtor) are checked in several parts of checkassignment(...). These checks are cheap, especially if none of the metamethods are implemented.

I will wait with the compositional aspect until we converge to well tested and supported solution.

[elliottslaughter]

I fixed this in #662 so hopefully those crashes will go away now.

[elliottslaughter]

And the FreeBSD build should also be fixed now.

[renehiemstra]

Fantastic! I'll have time to check this next week.

[renehiemstra]

Great! CI seems to be working.

[elliottslaughter]

I'll need to get back to this, but I left some initial comments.

Overall in terms of the code, it would be easier to review this if we either (a) removed formatting changes to terralib.lua or (b) applied those in a separate PR and then merged this PR on top of that one. I realize both approaches require effort but at the moment it's difficult to review simply due to the volume of whitespace changes we're talking about.

[elliottslaughter]

This is now a duplicate.

[renehiemstra]

Removed

[elliottslaughter]

If you put this file under docs/ then it will appear on the website once this PR is accepted, which I think would be nice.

I'm not sure how much it matters but I could suggest some formatting changes for the document itself:

• Capitalizing language names consistently: C++ not c++
• No need italicize language names: C++ not C++
• For code, types and function/method names, use code style not italics: std::string not std::string
• Have you run a spell checker?
• For code blocks, you can generalize use ``` by itself and do not need to also indent
• When you have a code block inside a list, I'm pretty sure GitHub's markdown is smart enough to make the code block part of the list item if you indent the entire block starting with the ``` line

I know there was quite a bit of effort that went into deciding this design, maybe that's worth documenting here too under a "Design decisions" section or similar?

[renehiemstra]

Improved documentation and added under docs/raii.md

[elliottslaughter]

This appears to be pass-by-value so is b not copied on being passed to bar?

[renehiemstra]

Yes, but terra performs shallow copies. Indeed, the example would also hold if passed by reference.

Honestly, it's a very bad code pattern that should not compile. I've removed it from the documentation. I'll add a section later on "don'ts"

[elliottslaughter]

I don't begrudge removing the trailing whitespace in most of this file, but this change seems to not follow the rest of the formatting of the code generally.

Is there a standardized code formatter you used to generate this formatting? We should probably talk about this.

I will say that the whitespace formatting changes do make the diffs rather hard to read, simply due to sheer volume. I'm not sure whether they can be easily backed out or applied separately. I suppose one option is to apply the format to the master branch first and rebase/merge this branch on top.

[renehiemstra]

I don't know exactly how that happened. I am using CLion for the first time, because it has debug capability for the C++ as well as the Lua side of the compiler stack. Maybe I pushed a wrong button.

The implementation has gone through quite some addition and shaving. Maybe it's good I close and reopen the pull request such that we get a clean commit history. That also gives me the change to review all the parts again myself and add some additional documentation here and there.

[renehiemstra]

reopened in #680