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(stdlibs): add math and math/bits #1153

Merged
merged 10 commits into from
Nov 23, 2023
Merged

feat(stdlibs): add math and math/bits #1153

merged 10 commits into from
Nov 23, 2023

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Sep 18, 2023

Great bug-finder package:

image

This adds packages math and math/bits, copied from go standard libraries with minimal modifications.

The changes from the standard libraries can be inspected with the following scripts:

cd path/to/stdlibs/math
for i in /usr/lib/go/src/math/*.go; do git diff --no-index "$i" "$(echo "$i" | sed 's#.*/\([^/]*\)\.go$#./\1.gno#g')"; done
cd bits
for i in /usr/lib/go/src/math/bits/*.go; do git diff --no-index "$i" "$(echo "$i" | sed 's#.*/\([^/]*\)\.go$#./\1.gno#g')"; done

These will generate a diff for each file in /usr/lib/src/go, and their gno counterpart in each directory.

Most changes involve removing the if haveXXXArch statements in the many functions which normally support being executed as native assembly instructions.

Additionally, within tests, all the dot imports present were removed (as they are not supported in Gno), and some bugs had to be worked around (I added references wherever this was the case).

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@thehowl thehowl added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related labels Sep 18, 2023
@thehowl thehowl requested review from jaekwon, moul and a team as code owners September 18, 2023 17:43
@thehowl thehowl self-assigned this Sep 18, 2023
@github-actions github-actions bot removed the 🧾 package/realm Tag used for new Realms or Packages. label Sep 18, 2023
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (902e678) 55.94% compared to head (d038ed8) 48.26%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
- Coverage   55.94%   48.26%   -7.68%     
==========================================
  Files         420      377      -43     
  Lines       65477    64130    -1347     
==========================================
- Hits        36631    30955    -5676     
- Misses      25983    30731    +4748     
+ Partials     2863     2444     -419     
Flag Coverage Δ
tm2-_test.flappy ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl
Copy link
Member Author

thehowl commented Oct 11, 2023

Draft until #1221

@thehowl thehowl marked this pull request as ready for review November 21, 2023 16:55
@thehowl
Copy link
Member Author

thehowl commented Nov 21, 2023

I've now cleaned up the code in the PR, since we merged #1221 :) Reviews welcome!

@deelawn
Copy link
Contributor

deelawn commented Nov 21, 2023

Can this block be removed as part of this PR?
https://github.com/gnolang/gno/blob/master/gnovm/tests/imports.go#L275-L282

Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Looks good. As noted in my other comment, I think it would be slightly more correct to include the following files inside _test suffixed packages:

  • all_test.gno
  • bits_test.gno
  • const_test.gno

I'm also happy to take care of these changes; just let me know.

@thehowl
Copy link
Member Author

thehowl commented Nov 23, 2023

@deelawn just took care of the changes you requested.

export_test.go needs to be in package math specifically (it exposes internal functions). arguably, we don't need this (as we don't use internal cpu-specific instructions, so there's no 2 different implementations for the same function), but I'd say we keep it for now to maintain similarity with the Go stdlib as much as possible.

If this looks good I think we can probably merge later today. :)

@gfanton gfanton merged commit 899a156 into master Nov 23, 2023
186 checks passed
@gfanton gfanton deleted the dev/morgan/stdlibs-math branch November 23, 2023 17:50
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: 🌟 Wanted for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants