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

WIP fix: handle assignments to dereferenced pointer values #1501

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Jan 7, 2024

Alternative fix for #1398

fixes #1326

@jaekwon jaekwon requested review from moul and a team as code owners January 7, 2024 06:32
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jan 7, 2024
@@ -0,0 +1,75 @@
# Reproducible Test for https://github.com/gnolang/gno/issues/1167
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad you tried txtar-based tests. How was your experience? Do you have any suggestions for improvement? If yes, please refer to #1269.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't me, I just copied this: https://gist.github.com/thehowl/e36b0f0d652a2a348a2fcd331a310417. Added a comment there. We should avoid using environment variables for production. Other than that I'm liking it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main thing with tests is this:

I just want "make test" to work as expected, at the root, and at the individual project levels. At the root it should just pass any options and call make test individually.

Anyone who pulls the codebase should be able to get make test passing, and go test ./... should all work as expected too.

Copy link
Member

Choose a reason for hiding this comment

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

My main thing with tests is this:

I just want "make test" to work as expected, at the root, and at the individual project levels. At the root it should just pass any options and call make test individually.

Anyone who pulls the codebase should be able to get make test passing, and go test ./... should all work as expected too.

That is the case!

@moul moul marked this pull request as draft January 8, 2024 17:20
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 134 lines in your changes are missing coverage. Please review.

Comparison is base (b525e8b) 56.08% compared to head (0e6e0db) 55.75%.
Report is 6 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/values.go 27.95% 115 Missing and 1 partial ⚠️
gnovm/pkg/gnolang/realm.go 26.66% 9 Missing and 2 partials ⚠️
gnovm/pkg/gnolang/ownership.go 42.85% 4 Missing ⚠️
gnovm/pkg/gnolang/op_exec.go 62.50% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   56.08%   55.75%   -0.34%     
==========================================
  Files         432      431       -1     
  Lines       66000    66639     +639     
==========================================
+ Hits        37016    37153     +137     
- Misses      26095    26578     +483     
- Partials     2889     2908      +19     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

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.

// unrefCopy makes a copy of the underlying value in the case of reference values.
// It copies other values as expected using the normal Copy method.
// XXX DEPRECATED (Copy() now does this).
// unrefCopy performs a copy but first unrefs if ptr.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

XXX not true... still need to resolve this...

// constructed of the correct size.
// This is more expensive, and should only be called for :=
// define and var decls.
func (pv PointerValue) Assign3(alloc *Allocator, store Store, rlm *Realm, tv2 TypedValue, cu bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: not called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: No status
Status: No status
Status: Triage
Development

Successfully merging this pull request may close these issues.

unexpected unreal object when assigning a local variable to a global variable (pointer)
2 participants