Page MenuHomePhabricator

eolian: evaluate our type ownership system and find out if it covers all cases and is generally sufficient
Closed, ResolvedPublic

Description

Not yet entirely sure how to handle this one. Is @owned fine as it is? Any feedback from bindings people on whether and how it could be improved to aid generators?

q66 created this task.Nov 14 2018, 5:15 AM
q66 moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

Its not really a problem, however, we should have a clarififaction. Lets say we have this:

class B {}

struct A {
   b : B @owned;
   i : int;
}

how we have a function call that does:

test {
   params {
      test : A;
   }
}

What does that imply ? That A is *not* owned, however, field b of A is owned ?

I understand @owned when applied to parameters and return values. I have never seen anything like that applied to a structure field. Does Eolian allow that?

q66 added a comment.Jan 18 2019, 8:34 AM

Yes, we allow that. It means the struct owns the reference. If the field is not @owned, it will not be freed when freeing the structure. It is separate from owning on params; if the param is not owned, it means the caller will free it, if it is, it means the callee will free it. Either way, @owned fields will be freed by one of them.

Your last sentence is sounding a bit ambiguous. Who is now in charge of freeing a owned field ? The Callee ? If so, then the fallback generation code is lacking exactly this.

q66 added a comment.Jan 19 2019, 8:03 AM

The thing in charge of freeing the structure is in charge of freeing its owned fields. If they're not marked owned, it is assumed that the struct does not own them and they will be freed somewhere else. I wonder if this behavior is what we want though. Maybe we should make them owned by default, and reverse the behavior so that you explicitly have to mark the field *not* owned in order to prevent it from being freed as a part of the structure?

I think it is okay to say that we can keep it like it is right now. Everything else is quite unachivable with the amount of API we have, and considering the amount of investigation this would require. Also - something not beeing freed is not perfect but the app keeps running, a double free is a crash...

With this beeing said, and the latest patchset beeing merged, I would say this task is done once we have documented that somewhere how all this is working.

q66 closed this task as Resolved.Aug 29 2019, 6:29 AM

Closing this as the system is generally sound. Syntax changes may happen but no design alterations