Re: [lucy-dev] [GitHub] lucy-clownfish pull request: CLOWNFISH-50 Convert some Obj methods...

[Marvin Humphrey]

On Wed, May 27, 2015 at 9:45 AM, nwellnhof <g...@git.apache.org> wrote:
> GitHub user nwellnhof opened a pull request:
>
>     https://github.com/apache/lucy-clownfish/pull/21
>
>     CLOWNFISH-50 Convert some Obj methods to inert functions
>
>     Make Obj_Get_Class, Obj_Get_Class_Name, and Obj_Is_A inert.
>
>     Changes to the Go bindings are untested.
>
>
> You can merge this pull request into a Git repository by running:
>
>     $ git pull https://github.com/nwellnhof/lucy-clownfish 
> CLOWNFISH-50-obj-method-to-inert
>
> Alternatively you can review and apply these changes as the patch at:
>
>     https://github.com/apache/lucy-clownfish/pull/21.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
>     This closes #21
>
> ----
> commit 6a8087c889bfb24db6dbc4aa306cf6d34b4d0423
> Author: Nick Wellnhofer <wellnho...@aevum.de>
> Date:   2015-05-27T15:23:59Z
>
>     Make Obj_Get_Class inert
>
> commit 2bec8bc4fa14f587ddc5b5e1ed52f6ff7ccd3d4b
> Author: Nick Wellnhofer <wellnho...@aevum.de>
> Date:   2015-05-27T15:47:31Z
>
>     Make Obj_Get_Class_Name inert
>
> commit fb12719d11db946293900a688ac3547e6ab75f17
> Author: Nick Wellnhofer <wellnho...@aevum.de>
> Date:   2015-05-27T16:34:31Z
>
>     Make Obj_Is_A inert
>
> ----

These patches look sound, but I think we will want to change some things -- +0
to merge.

In the Go bindings, I think it would be best not to implement these as methods
in the Obj interface, but instead as ordinary funcs.  The Obj interface is
already very large from a Go perspective.  See patch below my sig.

In the generated C headers, I think we should consider generating per-class
static inline wrappers instead of requiring users to cast.  For example, if we
generate this code...

    static CHY_INLINE bool
    LUCY_IXSEARCHER_IS_A(lucy_IndexSearcher *self, cfish_Class *ancestor) {
        return cfish_Obj_is_a((cfish_Obj*)self, ancestor);
    }

    static CHY_INLINE cfish_Class*
    LUCY_IXSEARCHER_GET_CLASS(lucy_IndexSearcher *self) {
        return cfish_Obj_get_class((cfish_Obj*)self);
    }

    static CHY_INLINE cfish_String*
    LUCY_IXSEARCHER_GET_CLASS_NAME(lucy_IndexSearcher *self) {
        return cfish_Obj_get_class_name((cfish_Obj*)self);
    }

... then client code can be type-safe:

    String *class_name = IXSEARCHER_GET_CLASS_NAME(searcher); // no cast

Finally, I think we should change the signature for is_a so that both `self`
and `ancestor` are `nullable`, and document it to return false in either case.
That's already the behavior of the implementation.

In contrast, `get_class` and `get_class_name` already have the proper
behavior: it should not be valid to supply a NULL self, and they should always
succeed.

Marvin Humphrey

diff --git a/runtime/go/build.go b/runtime/go/build.go
index 655b201..4a182c7 100644
--- a/runtime/go/build.go
+++ b/runtime/go/build.go
@@ -137,9 +137,6 @@ func runCFC() {
 func specMethods(parcel *cfc.Parcel) {
  objBinding := cfc.NewGoClass(parcel, "Clownfish::Obj")
  objBinding.SpecMethod("", "TOPTR() uintptr")
- objBinding.SpecMethod("", "GetClass() Class")
- objBinding.SpecMethod("", "GetClassName() string")
- objBinding.SpecMethod("", "IsA() bool")
  objBinding.Register()

  errBinding := cfc.NewGoClass(parcel, "Clownfish::Err")
diff --git a/runtime/go/clownfish/clownfish.go
b/runtime/go/clownfish/clownfish.go
index 2c446b9..f8016cc 100644
--- a/runtime/go/clownfish/clownfish.go
+++ b/runtime/go/clownfish/clownfish.go
@@ -92,21 +92,21 @@ func (o *ObjIMP) TOPTR() uintptr {
  return o.ref
 }

-func (o *ObjIMP) GetClass() Class {
- cfObj := (*C.cfish_Obj)(unsafe.Pointer(o.ref))
+func GetClass(o Obj) Class {
+ cfObj := (*C.cfish_Obj)(unsafe.Pointer(o.TOPTR()))
  class := C.cfish_Obj_get_class(cfObj)
  return WRAPClass(unsafe.Pointer(class))
 }

-func (o *ObjIMP) GetClassName() string {
- cfObj := (*C.cfish_Obj)(unsafe.Pointer(o.ref))
+func GetClassName(o Obj) string {
+ cfObj := (*C.cfish_Obj)(unsafe.Pointer(o.TOPTR()))
  className := C.cfish_Obj_get_class_name(cfObj)
  return CFStringToGo(unsafe.Pointer(className))
 }

-func (o *ObjIMP) IsA(class Class) bool {
- cfObj := (*C.cfish_Obj)(unsafe.Pointer(o.ref))
- cfClass := (*C.cfish_Class)(unsafe.Pointer(class.ref))
+func IsA(o Obj, class Class) bool {
+ cfObj := (*C.cfish_Obj)(unsafe.Pointer(o.TOPTR()))
+ cfClass := (*C.cfish_Class)(unsafe.Pointer(class.TOPTR()))
  retvalCF := C.cfish_Obj_is_a(cfObj, cfClass)
  return bool(retvalCF)
 }