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

Int8/16/32/64 documentation and unit tests #475

Merged
merged 45 commits into from
Jan 5, 2023

Conversation

luc-blaeser
Copy link
Contributor

@luc-blaeser luc-blaeser commented Dec 21, 2022

Improve Int8.mo, Int16.mo, Int32.mo, and Int64.mo:

Possible improvements for Int8/16/32/64 (#476):

  • Minimum value and maximum value constants are missing.
  • bitnot() has a superfluous second argument: bitnot(x, y) should be bitnot(x).
  • Support matchers with Int8/16/32/64/Nat8/16/32/64/Float/Order Testable.
  • Inconsistent naming of functions, e.g. bitrotLeft (instead of bitRotLeft).
  • Inconsistent type of second argument in bit operations: bitshiftLeft uses Int64 but bittest uses Nat.

rvanasa and others added 2 commits January 2, 2023 20:57
* Text module documentation and examples, first pass

* Clarify 'replace()' example

* Fix 'translate()' example

* Implement suggestions

* Replace 'assert' with 'print()'

* Update src/Text.mo

Co-authored-by: Kento Sugama <[email protected]>

* Apply suggestions from code review

Co-authored-by: Claudio Russo <[email protected]>

* Adjust example formatting

* Apply suggestions from code review

Co-authored-by: Claudio Russo <[email protected]>

Co-authored-by: Kento Sugama <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
* chore: add examples for CertifiedData

* chore: add link to example

* Apply suggestions from code review

Co-authored-by: Kento Sugama <[email protected]>

Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Kento Sugama <[email protected]>
src/Int16.mo Outdated Show resolved Hide resolved
src/Int8.mo Outdated Show resolved Hide resolved
src/Int8.mo Outdated Show resolved Hide resolved
src/Int32.mo Outdated Show resolved Hide resolved
src/Int64.mo Outdated Show resolved Hide resolved
@crusso
Copy link
Contributor

crusso commented Jan 3, 2023

Great stuff - added a few nits - any reason it's still in draft?

rvanasa and others added 10 commits January 3, 2023 13:03
* Add examples to ExperimentalStableMemory

* Add 'no-repl' specifiers to examples

* Clarify zero-initialization for 'grow()'

* 'no-repl' on top-level import example

* Update src/ExperimentalStableMemory.mo

Co-authored-by: Claudio Russo <[email protected]>

* Update src/ExperimentalStableMemory.mo

Co-authored-by: Claudio Russo <[email protected]>

Co-authored-by: Claudio Russo <[email protected]>
Improve `Float.mo`:
* Documentation
* Unit tests

Issues detected while refactoring:
* dfinity/motoko#3646
* dfinity/motoko#3647

Other possible areas of improvements for `Float`:
* `isNaN()` function is missing
* `equals()` comparison should consider numerical errors, by requiring an epsilon argument.
* Positive infinity and negative infinity constants are missing.
* `compare` function does not have a total order and is inconsistent, e.g. `compare(NaN, NaN) == #greater`. 
* `neq` function has a typo. Rename to `neg`

The suggestions for improvement are summarized in issue #456
Improve `Deque.mo`:
* Documentation
* Unit tests

Issue detected while refactoring:
* #464

Possible improvement for `Deque`:
* Reduce runtime and space performance of push/pop functions to `O(1)` worst-case per single function call (by avoiding List-split implementation).
Improve `RBTree.mo`:
* Documentation
* Unit tests

Possible improvements for `RBTree` (#492):
* Reduce garbage creation, especially during reading functions, such as `get()` and iteration.
* More efficient `size()` function.
Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
Co-authored-by: Claudio Russo <[email protected]>
@luc-blaeser luc-blaeser marked this pull request as ready for review January 5, 2023 09:59
@luc-blaeser
Copy link
Contributor Author

luc-blaeser commented Jan 5, 2023

Great stuff - added a few nits - any reason it's still in draft?

Thanks for the review. Sorry, I forgot to change the PR status. I changed it now - ready for approval or further improvement.

Fix some issues reported in #476:
* Offer minimum and maximum integer constants
* Remove superfluous second argument in `bitnot()`
@crusso
Copy link
Contributor

crusso commented Jan 5, 2023

Oh weird, why are there so many files changed now? Is that because of @ggreif's force-push?

Are there fewer diffs if you update the branch with master?

@ggreif
Copy link
Contributor

ggreif commented Jan 5, 2023

Are there fewer diffs if you update the branch with master?

Indeed, we are back to 8.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

This is great! Very thorough indeed!

@luc-blaeser luc-blaeser merged commit 4f8d37b into master Jan 5, 2023
@luc-blaeser luc-blaeser deleted the luc/improve-int-8-16-32-64-tests-doc branch January 5, 2023 15:48
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.

7 participants