Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kill the struct wrapper around Client #279

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Jul 31, 2022

...in generated interface types. This means that Client and the wrapper
types have the same underlying type, so we can CapList for both.

Notes:

  • The pogs package has some test failures that need fixing.
  • Annoyingly, we can't just use ~Client as the constraint, since the
    proper underlying type is struct { *client }. But it works.

Marking this as a draft for now.

...in generated interface types. This means that Client and the wrapper
types have the same underlying type, so we can CapList for both.

Notes:

- The pogs package has some test failures that need fixing.
- Annoyingly, we can't just use ~Client as the constraint, since the
  proper underlying type is `struct { *client }`. But it works.
@zenhack zenhack marked this pull request as draft July 31, 2022 01:58
lthibault
lthibault previously approved these changes Aug 1, 2022
@zenhack zenhack marked this pull request as ready for review August 1, 2022 19:55
@zenhack zenhack requested a review from lthibault August 1, 2022 19:55
lthibault
lthibault previously approved these changes Aug 1, 2022
@zenhack
Copy link
Contributor Author

zenhack commented Aug 1, 2022

Fixed the tests. One thing I really don't like about this is that go doc shows the constraint as ~struct { *client }, which is exposing implementation detail. And if you do go doc Client, it doesn't tell you that the underlying type is the same.

@zenhack zenhack changed the title WIP: Kill the struct wrapper around Client Kill the struct wrapper around Client Aug 1, 2022
@lthibault
Copy link
Collaborator

Forgive me if we've covered this before, but is there a reason why the constraint can't be ~Client?

@zenhack
Copy link
Contributor Author

zenhack commented Aug 1, 2022

If I use ~Client, I get:

./list.go:1151:16: invalid use of ~ (underlying type of Client is struct{*client})

This lets us refer to it outside the main package, and it also means
that CapList's constraint isn't exposing as much implementation detail.
@zenhack
Copy link
Contributor Author

zenhack commented Aug 1, 2022

Per discussion via matrix, I pushed another commit that makes an alias type ClientKind = ~struct { *client }, which makes this a bit better.

@zenhack zenhack requested a review from lthibault August 1, 2022 20:19
lthibault
lthibault previously approved these changes Aug 1, 2022
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue inline, otherwise LGMT.

pogs/pogs_test.go Outdated Show resolved Hide resolved
@zenhack zenhack requested a review from lthibault August 1, 2022 20:39
@lthibault lthibault merged commit fe8fc11 into capnproto:main Aug 1, 2022
@zenhack zenhack deleted the kill-client-struct-wrappers branch August 1, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants