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

Suggestion: Change CID type so it can be passed around directly and not by pointer #38

Closed
kevina opened this issue Oct 20, 2017 · 2 comments

Comments

@kevina
Copy link
Contributor

kevina commented Oct 20, 2017

Right now the Cid type is:

type Cid struct {
    version uint64
    codec   uint64
    hash    mh.Multihash
}

The internals are not exposed but it is expected to be passed around by pointer which in a way exposes some of the implementation details. For example in #3 I suggested we use a more compact structural representation and I think @Stebalien is suggesting to a serialized string instead. The best representation may be debatable, but either change will require that the Cid no longer be passed around by a pointer. We may even decide that a Cid is better represented by an interface and some point.

So for now may I suggestion that we start passing around the Cid type directly and change the representation to.

type Cid struct {
    *cid
} 
type cid struct {
    version uint64
    codec   uint64
    hash    mh.Multihash
}

This will then give us the freedom to try different internal representations without making requiring any API changes.

@whyrusleeping @Stebalien thoughts?

@Stebalien
Copy link
Member

From my experience today trying to replace Multiaddr with a bare type, this will be fun as we probably rely on the fact that Cids are nullable all over the place (although I agree we should probably do this anyways).

We could also just go straight to an interface.

Note: For my usecase, it's actually fine to continue passing it around as a pointer as long as I can cheaply extract some key to use in a map (either by dereferencing it or pulling out some field).

@kevina
Copy link
Contributor Author

kevina commented Oct 20, 2017

this will be fun

If we decide to go though with this I can likely do most of the work and probably in a day or two. I will need someone to help me with using gx-workspace, though, for handling the dependency updates.

probably rely on the fact that Cids are nullable all over the place

Maybe, in the cases where this is true we can use a pointer or we can have a special "empty" CID value. We won't know how bad this is until we try.

We could also just go straight to an interface.

I would be against this, it adds an extra level of indirection that I was trying to get ride of. I also think it will be overkill in this case.

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

No branches or pull requests

2 participants