Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Miscellaneous #121
Miscellaneous #121
Changes from 16 commits
5324608
639c8b1
eec5f46
ddad2b5
0341b00
2e653f5
46fbaed
dc2c6fa
01439d3
5b53de1
76f6347
bc25c0b
40e261b
0477b33
7cca0af
a026ed2
3334f44
84b166e
fd7e4c2
cc2e171
db8bfa1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I guess we may need to make the Prim.setCertifiedData take a argument too to make this really safe (otherwise people can directly call Prim.setCertifiedData).
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.
Doing this change may also break some third party libraries. But I guess we need to make break some eggs...
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.
This is probably worth doing, since otherwise there isn't much of a benefit to changing the base library if people can go around it by importing
Prim
directly.I'll roll this back to
set : (data : Blob) -> () = Prim.setCertifiedData
so that it preserves whether the prim function has the<system>
requirement.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.
@luc-blaeser are you ok with renaming equalWIthin (and notEqualWithin) to equal and notEqual?
Also another option might be to make both of these curried functions, that take the epsilon as a first argument and return an equality/inequality function of the usual binary type:
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.
The motivation here is for API uniformity. I suppose we could curry the arguments, although I think
equal(x, y, epsilon)
is more consistent with how we handle thecompare
andequal
functions for data structures.On the other hand, it could be worth doing something different to reduce possible confusion about the argument order, given the
equal(Float, Float, Float)
signature.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.
Right, I thinking about the situation where you want to pass the equality function as a first-class argument (which is why we define it for convenience...)
I don't have a strong opinion...
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.
Do we even want
notEqual
, since its the literal negation ofequal
@luc-blaeser and we don't providenotEqual
for other primitive types.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.
Seems reasonable to remove this function. I'll merge and open another PR to start working on the merge conflicts in parallel.
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.
If we aren't going to even provide any hash based data structures (are we?) I'd be tempted to get rid of this whole module
Hash.mo
.length
seemes misnamed (should be maxBitIndex or something) andbit
is probably an implementation detail ofTrie.mo
which uses the hash bits to index into a radix tree (IIRC). Might be worth checking if pos/length are used anywhere else.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.
Opened a separate PR for this: #129.