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

corona10 · 2017-07-25T10:20:26Z

Pass hi.go
Support unnamed types.

This PR should be reviewd after #120 is merged.

sbinet

(some of my comments are the same than for #120. I hope I have kept them consistent across the two PRs)

sbinet · 2017-07-31T11:37: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-31T11:40:10Z

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-31T11:42:58Z

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-31T11:43:38Z

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 genCdefType.

sbinet · 2017-07-31T11:44:51Z

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:18:11Z

bind/gencffi_type.go

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


sbinet · 2017-07-31T12:18:21Z

bind/gencffi_type.go

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


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

bind/gencffi_type.go

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


sbinet · 2017-07-31T12:19:12Z

bind/gencffi_type.go

+		g.wrapper.Printf("_cffi_helper.lib.cgo_func_%[1]s_ass_item(self.cgopy, idx, pyitem)\n", sym.id)
+	default:
+		panic(fmt.Errorf(
+			"gopy: init for %s not handled",


looks like a copy paste from __init__.

sbinet · 2017-07-31T12:21:58Z

bind/symtab.go

@@ -130,6 +130,10 @@ func (s symbol) isMap() bool {
 	return (s.kind & skMap) != 0
 }

+func (s symbol) isSequence() bool {


is this a function that tests for the Go-concept of a sequence or for the python concept of a sequence?

in other words, do we want to also test for maps ?

a quick grep in the code seems to indicate isSequence is used to determine whether we need to generate methods that implement the python's sequence protocol...

a quick grep in the code seems to indicate isSequence is used to determine whether we need to generate methods that implement the python's sequence protocol...

Yes, that is. :)

in other words, do we want to also test for maps?

Yes, we need to generate protocols for map also. I am investigating about that.

@sbinet isIterable might be better?

I think isPySequence better conveys what piece of information we want to extract and its later use.

corona10 · 2017-08-03T11:31:21Z

@sbinet Updated!

corona10 · 2017-08-03T16:39:56Z

@sbinet Done PTAL!

sbinet · 2017-08-04T07:46:04Z

bind/gencffi_type.go

+	g.wrapper.Indent()
+	g.wrapper.Printf("if idx >= len(self):\n")
+	g.wrapper.Indent()
+	g.wrapper.Printf("raise IndexError('array index out of range')\n")


would it be possible to raise "array index ..." when it's a Go array and "slice index ..." when it's a Go slice ?

@sbinet That looks great I will update it.

sbinet · 2017-08-04T07:47:51Z

bind/gencffi_type.go

+	g.wrapper.Indent()
+	g.wrapper.Printf("if idx >= len(self):\n")
+	g.wrapper.Indent()
+	g.wrapper.Printf("raise IndexError('array index out of range')\n")


ditto: it would be much nicer to raise an error with the correct type name (array/slice).

sbinet · 2017-08-04T07:48:58Z

main_test.go

+--- p.Salary(2): 20
+--- p.Salary(24): caught: can't work for 24 hours!
+--- Person.__init__
+caught: invalid type for '%!s(BADINDEX)' attribute | err-type: <type 'exceptions.TypeError'>


that seems like an error somewhere in our generated code...

sbinet · 2017-08-04T07:49:40Z

bind/gencffi_struct.go

@@ -103,6 +104,10 @@ func (g *cffiGen) genStructInit(s Struct) {
 		g.wrapper.Outdent()
 		g.wrapper.Printf("if %[1]s != None:\n", kwd_name)
 		g.wrapper.Indent()
+		g.wrapper.Printf("if not isinstance(%[1]s, %[2]s):\n", kwd_name, ifield.sym.pysig)
+		g.wrapper.Indent()
+		g.wrapper.Printf("raise TypeError(\"invalid type for '%[2]s' attribute\")\n", field.Name())


the index should be %[1]s instead.

* Pass hi.go * Support unnamed types.

corona10 · 2017-08-04T11:12:25Z

@sbinet Updated and sorry for some mistakes.

sbinet · 2017-08-04T14:42:15Z

main_test.go

+slice[2]: caught: slice index out of range
+slice: []int{1, 42}
+len(slice): 2
+mem(slice): caught: cannot make memory view because object does not have the buffer interface


ok.
please file an issue so we don't forget to implement the buffer interface for slices.

sbinet · 2017-08-04T14:42:26Z

main_test.go

+arr[2]: caught: array index out of range
+arr: [2]int{1, 42}
+len(arr): 2
+mem(arr): caught: cannot make memory view because object does not have the buffer interface


ok.
please file an issue so we don't forget to implement the buffer interface for arrays.

corona10 force-pushed the gsoc-unnamed-hi branch from ad9bfe1 to 8a8402b Compare July 25, 2017 17:27

sbinet requested changes Jul 31, 2017

View reviewed changes

corona10 force-pushed the gsoc-unnamed-hi branch 3 times, most recently from b36e491 to 015efae Compare August 3, 2017 11:13

corona10 changed the title ~~[WIP] cffi: Support unnamed types and pass hi.go~~ cffi: Support unnamed types and pass hi.go Aug 3, 2017

corona10 force-pushed the gsoc-unnamed-hi branch 2 times, most recently from 1bfcf1b to 4fcfc48 Compare August 3, 2017 16:29

sbinet reviewed Aug 4, 2017

View reviewed changes

corona10 force-pushed the gsoc-unnamed-hi branch 3 times, most recently from 597d4e8 to fc53bee Compare August 4, 2017 10:08

cffi: Support unnamed types and pass hi.go

2ec1fd9

* Pass hi.go * Support unnamed types.

corona10 force-pushed the gsoc-unnamed-hi branch from fc53bee to 2ec1fd9 Compare August 4, 2017 10:47

sbinet reviewed Aug 4, 2017

View reviewed changes

sbinet approved these changes Aug 4, 2017

View reviewed changes

corona10 mentioned this pull request Aug 4, 2017

cffi: Implement the buffer interface for slices and arrays. #125

Closed

sbinet merged commit 4eac7b0 into go-python:master Aug 4, 2017

corona10 deleted the gsoc-unnamed-hi branch August 4, 2017 15:18

		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 unnamed types and pass hi.go #123

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

Uh oh!

Conversation

corona10 commented Jul 25, 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 Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 3, 2017

Uh oh!

corona10 commented Aug 3, 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!

corona10 commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10 Aug 3, 2017 •

edited

Loading