cffi: Support named type + Pass 'seq.go' test. #120

corona10 · 2017-07-15T16:51:54Z

Pass 'named.go' test.
Support to generate named type includes slice and array types.
Add doc.go for bind package.
Add comments.
Support iadd for Go slice types.
Add a 'seq.go' test for the CFFI engine.

corona10 · 2017-07-17T07:11:56Z

@alclaude Ready for review :)

sbinet · 2017-07-31T12:26:05Z

bind/doc.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+/*


Standard documentation for packages look like:

// Package bind provides tools to generate bindings to use Go from Python. package bind

sbinet · 2017-07-31T12:26:23Z

bind/doc.go

+to call Go code and pass objects from Python.
+
+*/
+


remove that empty line (otherwise the documentation isn't attached to the package)

sbinet · 2017-07-31T12:26:48Z

bind/gencffi_cdef.go

 	"go/types"
 	"strconv"
 	"strings"
 )

+// Generate C definitions of Go struct.


the canonical way to document functions/methods is:

// genCdefStruct generates C definitions for a Go struct. func (g *cffiGen) genCdefStruct(s Struct) {

(ie: // [method or function name] [verb] [stuff...])

sbinet · 2017-07-31T12:27:13Z

bind/gencffi_cdef.go

 func (g *cffiGen) genCdefStruct(s Struct) {
 	g.wrapper.Printf("// A type definition of the %[1]s.%[2]s for wrapping.\n", s.Package().Name(), s.sym.cgoname)
 	g.wrapper.Printf("typedef void* %s;\n", s.sym.cgoname)
 	g.wrapper.Printf("extern void* cgo_func_%s_new();\n", s.sym.id)
 }

+// Generate C definitions of Go type.


please use the canonical way to document functions/methods.

sbinet · 2017-07-31T12:27:40Z

bind/gencffi_cdef.go

 func (g *cffiGen) genCdefType(sym *symbol) {
+	g.wrapper.Printf("// C definitions of the %[1]s.%[2]s.\n", g.pkg.pkg.Name(), sym.id)


I would rephrase as:

"// C definitions for Go type %[1]s.%[2]s.\n"

sbinet · 2017-07-31T12:41:15Z

bind/gencffi_type.go

+	case sym.isArray():
+		g.wrapper.Printf("if args:\n")
+		g.wrapper.Indent()
+		g.wrapper.Printf("if not isinstance(args[0], collections.Sequence):\n")


here, collections.Sequence is ok. (we need len(x))

sbinet · 2017-07-31T12:41:44Z

bind/gencffi_type.go

+	case sym.isSlice():
+		g.wrapper.Printf("if args:\n")
+		g.wrapper.Indent()
+		g.wrapper.Printf("if not isinstance(args[0], collections.Sequence):\n")


but here, I would use collections.Iterable instead.

sbinet · 2017-07-31T12:42:02Z

bind/gencffi_type.go

+	g.wrapper.Printf("\n")
+}
+
+// Generate Type converter between C and Python.


sbinet · 2017-07-31T12:42:07Z

bind/gencffi_type.go

+	g.wrapper.Printf("\n")
+}
+
+// Generate Type methods.


sbinet · 2017-07-31T12:42:14Z

bind/gencffi_type.go

+	g.wrapper.Printf("\n")
+}
+
+// Generate Type __str__ method.


corona10 · 2017-07-31T17:49:17Z

@sbinet Updated!!

sbinet

some more nitpicks.

sbinet · 2017-08-01T18:50:03Z

bind/doc.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package bind package generates language bindings that make it possible


typo: Package bind package

sbinet · 2017-08-01T18:51:17Z

bind/gencffi_cdef.go

 	"go/types"
 	"strconv"
 	"strings"
 )

+// genCdefStruct generate C definitions of Go struct.


s/generate/generates/
s/of Go/for a Go/

sbinet · 2017-08-01T18:51:40Z

bind/gencffi_cdef.go

 func (g *cffiGen) genCdefStruct(s Struct) {
 	g.wrapper.Printf("// A type definition of the %[1]s.%[2]s for wrapping.\n", s.Package().Name(), s.sym.cgoname)
 	g.wrapper.Printf("typedef void* %s;\n", s.sym.cgoname)
 	g.wrapper.Printf("extern void* cgo_func_%s_new();\n", s.sym.id)
 }

+// genCdefType generates C definitions of Go type.


s/of Go/for a Go/

sbinet · 2017-08-01T18:52:45Z

bind/gencffi_cdef.go

+	g.wrapper.Printf("\n")
+}
+
+// genTypeCdefInit generate cgo_func_XXXX_new() of Go type.


s/generate/generates/
s/of Go/for a Go/

sbinet · 2017-08-01T18:53:24Z

bind/gencffi_cdef.go

 	}
 }

+// genCdefFunc generates a C definition of Go function.


s/of Go/for a Go/

sbinet · 2017-08-01T18:56:24Z

bind/gencffi_cdef.go

@@ -99,6 +198,8 @@ func (g *cffiGen) genCdefMethod(f Func) {
 	g.wrapper.Printf("extern %[1]s cgo_func_%[2]s(%[3]s);\n", cdef_ret, f.id, paramString)
 }

+// genCdefStructMemberGetter generates
+// C definitions of Getter/Setter for Go struct.


s/for Go struct/for a Go struct/
also, please everything on one line (ie: no new line after generates.)

sbinet · 2017-08-01T18:56:57Z

bind/gencffi_cdef.go

@@ -116,6 +217,8 @@ func (g *cffiGen) genCdefStructMemberGetter(s Struct, i int, f types.Object) {
 	}
 }

+// genCdefStructMemberSetter generates
+// C defintion of Setter for Go struct.


s/for Go/for a Go/.
s/defintion/definition/
also: please everything on one line.

sbinet · 2017-08-01T18:58:04Z

bind/gencffi_cdef.go

@@ -126,6 +229,14 @@ func (g *cffiGen) genCdefStructMemberSetter(s Struct, i int, f types.Object) {
 	g.wrapper.Printf("extern void cgo_func_%[1]s_setter_%[2]d(void* p0, %[3]s p1);\n", s.sym.id, i+1, cdef_value)
 }

+// genCdefStructTPStr generates
+// C definitions of str method for Go struct.


s/for Go/for a Go/
also: please everything on one line.

sbinet · 2017-08-01T18:58:46Z

bind/gencffi_cdef.go

 func (g *cffiGen) genCdefStructTPStr(s Struct) {
 	g.wrapper.Printf("extern GoString cgo_func_%[1]s_str(void* p0);\n", s.sym.id)
 }
+
+// genCdefTypeTPStr generates
+// C definitions of str method for Go type.


s/for Go/for a Go/
also: please everything on one line.

sbinet · 2017-08-01T19:01:03Z

bind/gencffi_type.go

+	"go/types"
+)
+
+// genType generates Go type with Python wrapper class.


that doesn't read very well.
please replace with something along the lines of:

// genType generates a Python class wrapping the given Go type.

* Pass 'named.go' test. * Support to generate named type includes slice and array types. * Add doc.go for bind package. * Add comments. * Support __iadd__ for Go slice types. * Add a 'seq.go' test for the CFFI engine.

corona10 · 2017-08-02T15:46:03Z

@sbinet
Thank you for comment about all sentences.
The update was a little bit late due to unexpected event.

corona10 · 2017-08-02T20:36:46Z

Updated

corona10 force-pushed the gsoc-type branch 7 times, most recently from 05a8367 to 04f3705 Compare July 17, 2017 06:54

corona10 changed the title ~~[WIP] cffi: Generating named type.~~ cffi: Generate named type. Jul 17, 2017

corona10 changed the title ~~cffi: Generate named type.~~ cffi: Generate named types. Jul 17, 2017

corona10 force-pushed the gsoc-type branch from 04f3705 to e52c4d7 Compare July 17, 2017 07:01

corona10 force-pushed the gsoc-type branch from e52c4d7 to e0fd256 Compare July 21, 2017 08:56

corona10 changed the title ~~cffi: Generate named types.~~ cffi: Support named type + Pass 'seq.go' test. Jul 21, 2017

corona10 mentioned this pull request Jul 25, 2017

cffi: Support unnamed types and pass hi.go #123

Merged

sbinet requested changes Jul 31, 2017

View reviewed changes

corona10 changed the title ~~cffi: Support named type + Pass 'seq.go' test.~~ [WIP] cffi: Support named type + Pass 'seq.go' test. Jul 31, 2017

corona10 force-pushed the gsoc-type branch from e0fd256 to d5978ec Compare July 31, 2017 16:12

corona10 changed the title ~~[WIP] cffi: Support named type + Pass 'seq.go' test.~~ cffi: Support named type + Pass 'seq.go' test. Jul 31, 2017

sbinet requested changes Aug 1, 2017

View reviewed changes

cffi: Support named type + Pass 'seq.go' test.

c2676d7

* Pass 'named.go' test. * Support to generate named type includes slice and array types. * Add doc.go for bind package. * Add comments. * Support __iadd__ for Go slice types. * Add a 'seq.go' test for the CFFI engine.

corona10 force-pushed the gsoc-type branch from d5978ec to c2676d7 Compare August 2, 2017 15:43

sbinet approved these changes Aug 3, 2017

View reviewed changes

sbinet merged commit c2676d7 into go-python:master Aug 3, 2017

corona10 deleted the gsoc-type branch August 3, 2017 09:21

		func (g cffiGen) genCdefType(sym symbol) {
		g.wrapper.Printf("// C definitions of the %[1]s.%[2]s.\n", g.pkg.pkg.Name(), sym.id)

cffi: Support named type + Pass 'seq.go' test. #120

cffi: Support named type + Pass 'seq.go' test. #120

Uh oh!

Conversation

corona10 commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 commented Jul 31, 2017

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 commented Aug 2, 2017

Uh oh!

corona10 commented Aug 2, 2017

Uh oh!

Uh oh!

corona10 commented Jul 15, 2017 •

edited

Loading