My take on polymorphism #13

2013-04-16T16:20:07-05:00

vmg commented

2013-04-16 16:20:07 -05:00

(Migrated from github.com)

So, this is very close to what @carlosmn had. I just abstracted the common Object code into the object.go file and added a global helper that sets the finalizers for all allocations, so we can re-use it all over the code base.

@carlosmn, how do you feel about it?

So, this is very close to what @carlosmn had. I just abstracted the common `Object` code into the `object.go` file and added a global helper that sets the finalizers for all allocations, so we can re-use it all over the code base. @carlosmn, how do you feel about it?

carlosmn commented

2013-04-16 16:45:08 -05:00

(Migrated from github.com)

Looks good. I like lookupType.

I wonder if there's a way we can have one finalizer for all types. Underneath it's just git_object_free anyway.

Looks good. I like `lookupType`. I wonder if there's a way we can have one finalizer for all types. Underneath it's just `git_object_free` anyway.

vmg commented

2013-04-17 17:55:57 -05:00

(Migrated from github.com)

I wonder if there's a way we can have one finalizer for all types.

Like this?

> I wonder if there's a way we can have one finalizer for all types. Like this?

carlosmn commented

2013-04-18 03:14:11 -05:00

(Migrated from github.com)

Yeah, that's awesome, the common parts belong to the basic object.

Why did you remove the pointer receivers for the functions? Putting them back doesn't show any errors when compiling or running the tests.

Yeah, that's awesome, the common parts belong to the basic object. Why did you remove the pointer receivers for the functions? Putting them back doesn't show any errors when compiling or running the tests.

Merovius commented

2013-04-25 19:15:44 -05:00

(Migrated from github.com)

There is actually a deeper problem with removing the pointer-receivers. runtime.SetFinalizer expects a pointer-receiver, so having a value-receiver breaks stuff here (I actually get a runtime-error because of that here).

EDIT: Sorry, seem to be sleep-deprived, the Free()-methods still have pointers? Then the error has to be at another point…

EDIT2: No, it is not really. gitObject.Free() still has a value-receiver, as it seems, which reproducibly creates a crash when the runtime tries to garbage-collect an objec. Error is

runtime.SetFinalizer: first argument is git.gitObject, not pointer
fatal error: runtime.SetFinalizer

changing it to a pointer-receiver gets rid of the problem.

EDIT3: Created a (trivial) pull-request on polymorphism-take-2 to change it to a pointer-receiver

There is actually a deeper problem with removing the pointer-receivers. runtime.SetFinalizer expects a pointer-receiver, so having a value-receiver breaks stuff here (I actually get a runtime-error because of that here). EDIT: Sorry, seem to be sleep-deprived, the Free()-methods still have pointers? Then the error has to be at another point… EDIT2: No, it is not really. gitObject.Free() still has a value-receiver, as it seems, which reproducibly creates a crash when the runtime tries to garbage-collect an objec. Error is > runtime.SetFinalizer: first argument is git.gitObject, not pointer > fatal error: runtime.SetFinalizer changing it to a pointer-receiver gets rid of the problem. EDIT3: Created a (trivial) pull-request on polymorphism-take-2 to change it to a pointer-receiver

Merovius commented

2013-05-14 14:52:09 -05:00

(Migrated from github.com)

Not that my opinion will count much, but I like that change and would like to see it merged ;)

vmg commented

2013-06-13 12:16:46 -05:00

(Migrated from github.com)

Brilliant. Sorry it took me so long to go through the PR list, @Merovius. Got so much stuff on my plate right now.

Expect me to revisit these bindings and greatly expand them in about a month (I'm going to actually start using them). Until them, your PRs are always appreciated.

luv,
vmg

Brilliant. Sorry it took me so long to go through the PR list, @Merovius. Got so much stuff on my plate right now. Expect me to revisit these bindings and greatly expand them in about a month (I'm going to actually start using them). Until them, your PRs are always appreciated. luv, vmg

Sign in to join this conversation.

No reviewers