-
Notifications
You must be signed in to change notification settings - Fork 50
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
Conversation
There was a problem hiding this 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:
-
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? -
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 callingAsInt
/AsUint
. (in the normal case, the change is picked up, but in the uint case, the change is not) Does this matter?
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. |
a2c7491
to
d9c7f10
Compare
Updated this to #413 and changed it so that for all uint64 types, we return a new This should address both problems @hannahhoward pointed out with the earlier version. |
rolling this in to #414 |
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.