Pogs interfaces #66

zenhack · 2017-02-24T01:16:03Z

Addresses #62.

//cc @zombiezen, when you have the time.

zombiezen · 2017-03-20T15:54:02Z

pogs/extract.go

@@ -25,6 +25,14 @@ type extracter struct {
 	nodes nodemap.Map
 }

+var (


Don't really need the doc comment.

var clientType = reflect.TypeOf((*capnp.Client)(nil)).Elem()

zombiezen · 2017-03-20T15:57:30Z

pogs/extract.go

+		}
+		client := p.Interface().Client()
+		if client != nil {
+			if !val.Type().Implements(clientType) {


I think you actually want

if val.Type() != clientType {

zombiezen · 2017-03-20T15:59:52Z

pogs/extract.go

@@ -376,6 +397,15 @@ func isTypeMatch(r reflect.Type, s schema.Type) bool {
 	case schema.Type_Which_list:
 		e, _ := s.List().ElementType()
 		return r.Kind() == reflect.Slice && isTypeMatch(r.Elem(), e)
+	case schema.Type_Which_interface:
+		if !r.Implements(clientType) && r.Kind() == reflect.Struct {
+			field, ok := r.FieldByName("Client")


I'd like for this to check for a more strict bound:

Has one field

That field is called Client

That field is of type capnp.Client

zombiezen · 2017-03-20T16:00:36Z

pogs/insert.go

 	default:
 		return fmt.Errorf("unknown field type %v", typ.Which())
 	}
 	return nil
 }

+func getCapPtr(seg *capnp.Segment, val reflect.Value) capnp.Ptr {


nit: just capPtr

zombiezen · 2017-03-20T16:01:19Z

pogs/interface_test.go

+
+import (
+	"golang.org/x/net/context"
+	"testing"


nit: group stdlib packages apart from others

zombiezen · 2017-03-20T16:02:30Z

pogs/interface_test.go

+	Base EchoBase
+}
+
+func TestInsertIFace(t *testing.T) {


Any reason to not add these to the broader table test? I think I have better error handling in there.

Using reflect.DeepEqual on interfaces doesn't really do what we want in this case, and it seemed prudent to have something that tested the behavior of the interface, which would require special casing in the general test anyway. I think at the time I decided making it work would be more disruptive to the existing tests than really made sense. I'm willing to be convinced otherwise.

I've removed reflect.DeepEqual usage. WDYT now? 😃

@zombiezen, I think I've addressed everything besides this point. Any thoughts on my comment above?

@zombiezen, ping?

zombiezen · 2017-04-16T02:32:15Z

Sorry, lost track of this with other stuff. I'll take a look tomorrow.

zombiezen

Sorry, I forgot I had some comments queued up that I never sent.

zombiezen · 2017-03-23T21:14:11Z

pogs/interface_test.go

+	extractedBase := EchoBase{}
+	err = Extract(&extractedBase, air.EchoBase_TypeID, base.Struct)
+	checkFatal(t, "Extract", err)
+	if extractedBase.Echo.Client != nil {


I would start the Client off as non-nil and check that it got set to nil.

zombiezen · 2017-03-23T21:56:41Z

pogs/interface_test.go

+	Base EchoBase
+}
+
+func TestInsertIFace(t *testing.T) {


I've removed reflect.DeepEqual usage. WDYT now? 😃

zombiezen · 2017-04-17T19:35:37Z

pogs/extract.go

@@ -376,6 +391,24 @@ func isTypeMatch(r reflect.Type, s schema.Type) bool {
 	case schema.Type_Which_list:
 		e, _ := s.List().ElementType()
 		return r.Kind() == reflect.Slice && isTypeMatch(r.Elem(), e)
+	case schema.Type_Which_interface:
+		if r.Implements(clientType) {


I think you want r == clientType here.

So far just Insert, and I haven't tested this; it builds and the existing tests still pass, but I need to get some other work done, so I'm putting it down for the moment.

...and fix a corresponding bug.

This involved factoring out/moving a handful of things to global scope that were previously only visible from inside TestInsertIFace. Still TODO is handling things inside lists; this hasn't been tested and I suspect it doesn't work.

Passes. Note that this adds a type to the aircraft schema.

The only bit left is the question about moving the new tests into the big table, which I need to stare at a bit.

zenhack · 2017-04-18T01:01:58Z

I think I can make the tests work with the table; I'll have to add a bit of logic to actually call testEcho when the interface is (expectedly) not nil. So it won't be checking for equality really, but that's OK I think. I'll get to this soon.

I addressed your other two comments.

The last of the insert tests is actually failing right now, and thinking about it I'm not sure how it could be made to pass; It's not clear the wire format can represent that.

zenhack · 2017-04-19T04:07:53Z

@zombiezen, I added interface stuff to the main table based tests. It's actually failing currentlly in a case where I'm trying to insert a slice of structs, where some of them have a nil client and some don't; the resulting message ends up with them all filled in.

I bet I know what's going on: the composite list wire format can't actually represent lists of structs that have different numbers of pointers filled in, so there's an extra pointer at the end of some of the structs that's being interpreted as a capability pointer. CapNProto doesn't have a notion of a nil/null pointer, just an "absent" one, so I don't think there's a "correct" way to encode a list like this.

I'm not sure how to handle this case besides just mentioning in the docs that you can't do that. Thoughts?

zombiezen · 2017-04-28T17:58:27Z

I bet I know what's going on: the composite list wire format can't actually represent lists of structs that have different numbers of pointers filled in, so there's an extra pointer at the end of some of the structs that's being interpreted as a capability pointer.

But there is a null pointer for capabilities, this happens all over the place. It's the zero word. That seems like a legitimate failure to me. Any pointer type in Cap'n Proto can be null.

zenhack · 2017-04-28T18:03:45Z

Ah, you're right -- I missed that reading the spec. I will dig a bit deeper then.

zenhack · 2017-04-28T18:32:24Z

@zombiezen, fixed.

I hadn't been aware of this before. This also fixes the test failure; presumably I made some mistake in re-implementing the logic.

zombiezen · 2017-04-28T20:29:49Z

Thanks! Sorry this took a while, but I'm happy with how it turned out.

zenhack · 2017-04-29T00:30:39Z

Cool. No worries about the delays; things come up.

zombiezen suggested changes Mar 20, 2017

View reviewed changes

zenhack force-pushed the pogs-interfaces branch from 2a89c4c to a95b7ef Compare March 20, 2017 18:22

zombiezen reviewed Apr 17, 2017

View reviewed changes

zenhack added 11 commits April 17, 2017 20:55

WIP: support interface types in pogs.

3f2f2ab

So far just Insert, and I haven't tested this; it builds and the existing tests still pass, but I need to get some other work done, so I'm putting it down for the moment.

Write tests for Insert & interfaces.

bbeaf97

...and fix a corresponding bug.

Update docs to describe interface mapping

1cb51d4

Implement support for interfaces in Extract

65daf2d

This involved factoring out/moving a handful of things to global scope that were previously only visible from inside TestInsertIFace. Still TODO is handling things inside lists; this hasn't been tested and I suspect it doesn't work.

Don't panic if the message contains nil interfaces.

fe816f1

Add test for inserting lists w/ interfaces.

48ddd2b

Passes. Note that this adds a type to the aircraft schema.

Test Extract on a type with lists & interfaces

80e03ee

Insert: don't panic when passed nil Clients

6dfdedd

Incorporate most of @zombiezen's feedback.

ab55460

The only bit left is the question about moving the new tests into the big table, which I need to stare at a bit.

Address more of @zombiezen's feeback

612c67f

Set clients to nil if the fields are nil in the message.

d1ca975

zenhack force-pushed the pogs-interfaces branch from a95b7ef to d1ca975 Compare April 18, 2017 01:00

pogs tests: integrate the interface tests into the main tables.

54e8d3e

The last of the insert tests is actually failing right now, and thinking about it I'm not sure how it could be made to pass; It's not clear the wire format can represent that.

re-use listeq function.

75b4aac

I hadn't been aware of this before. This also fixes the test failure; presumably I made some mistake in re-implementing the logic.

zombiezen approved these changes Apr 28, 2017

View reviewed changes

zombiezen merged commit 917bd4b into capnproto:master Apr 28, 2017

zenhack mentioned this pull request May 6, 2017

pogs: support interface types #62

Closed

zenhack deleted the pogs-interfaces branch April 3, 2022 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pogs interfaces #66

Pogs interfaces #66

zenhack commented Feb 24, 2017

zombiezen Mar 20, 2017

zombiezen Mar 20, 2017

zombiezen Mar 20, 2017

zombiezen Mar 20, 2017

zombiezen Mar 20, 2017

zombiezen Mar 20, 2017

zenhack Mar 20, 2017

zombiezen Mar 23, 2017

zenhack Mar 28, 2017

zenhack Apr 16, 2017

zombiezen commented Apr 16, 2017

zombiezen left a comment

zombiezen Mar 23, 2017

zombiezen Mar 23, 2017

zombiezen Apr 17, 2017

zenhack commented Apr 18, 2017 •

edited

Loading

zenhack commented Apr 19, 2017

zombiezen commented Apr 28, 2017

zenhack commented Apr 28, 2017

zenhack commented Apr 28, 2017

zombiezen commented Apr 28, 2017

zenhack commented Apr 29, 2017

Pogs interfaces #66

Pogs interfaces #66

Conversation

zenhack commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zombiezen commented Apr 16, 2017

zombiezen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenhack commented Apr 18, 2017 • edited Loading

zenhack commented Apr 19, 2017

zombiezen commented Apr 28, 2017

zenhack commented Apr 28, 2017

zenhack commented Apr 28, 2017

zombiezen commented Apr 28, 2017

zenhack commented Apr 29, 2017

zenhack commented Apr 18, 2017 •

edited

Loading