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

feat(bindnode): support full uint64 range #429

Closed
wants to merge 6 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 24, 2022

This is a little bit awkward because it's a PR against #414 but pulls in #413 to get the uint64 range in and out of dag-cbor. So the first two commits are from #413 and the HEAD is just the extra bits required to wire up bindnode to handle uint64 values.

Marking as Draft for now but this is proving that #413 has real utility. And, we may need it sooner than later for some Filecoin types.

@rvagg rvagg self-assigned this May 24, 2022
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Two possible concerns, ranked in order of importance:

  1. The use of uint only for >math.MaxInt64 seems confusing. If I have a uint64 value wrapped in bindnode, I will sometimes get a node that satisfies the UintNode interface, while other times I will not, depending on the underlying value being >math.MaxInt64. I guess it doesn't matter for serialization. But it also seems weird for other use cases?

  2. It appears the normal path for bindnode may contain an underlying pointer type that isn't converted to a value type until you call one of the As methods -- i.e. AsInt. However, the uint64 type converts to value as soon as its constructed. This could result in different behavior if the value the underlying pointer points to changes between construction and calling AsInt/AsUint. (in the normal case, the change is picked up, but in the uint case, the change is not) Does this matter?

@rvagg
Copy link
Member Author

rvagg commented May 30, 2022

  1. Yes, that inconsistency could end up in theoretically weird cases later on and maybe consistency is more valuable in this case—with bindnode at least we know that the underlying type is >=0; the rest of go-ipld-prime doesn't have this information, even typed nodes don't get that kind of info, but with bindnode we're supplying additional metadata in the Go types so maybe we should be using that?
  2. Maybe? I was trying to avoid complexity here but maybe there's another case for consistency here since, as you point out, there may be situations where you get different behaviour.

Will think on this more, for now this is working for what I need but we are going to need it properly resolved in a satisfactory way.

I'm also thinking of getting rid of this "positive" bool on uints, will comment over in #413 about that.

@rvagg
Copy link
Member Author

rvagg commented May 31, 2022

Updated this to #413 and changed it so that for all uint64 types, we return a new _uintNode that is only used for this case - it does AsInt() in the same way as _node (so custom converters will still work), and it does the new AsUint() but doesn't retrieve the value until then, so we don't have the problem of potentially capturing the value too early and having it change underneath us. We also have the consistency of this happening for every uint64 regardless of value. Other uint* types still use _node.

This should address both problems @hannahhoward pointed out with the earlier version.

@rvagg rvagg marked this pull request as ready for review May 31, 2022 11:24
@rvagg
Copy link
Member Author

rvagg commented Jun 10, 2022

rolling this in to #414

@rvagg rvagg closed this Jun 10, 2022
@rvagg rvagg deleted the rvagg/bindnode-uint64 branch June 10, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

2 participants