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

Replace PCtx parameters with TCtx in VM code #252

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Mar 13, 2022

  • Where TCtx is now used, also use func instead of proc, if applicable

Not all usages of PCtx are replaced. PCtx is still used in procedures
that form the border between non-VM/VM code. vmops also still uses
PCtx, as some ops need to capture a reference to the context

compiler/vm/vm.nim Outdated Show resolved Hide resolved
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Minor item, the other thing I noticed is that some functions have effects, will need to change those to procs or break them up. I'd go the proc route, personally.

compiler/vm/vmdef.nim Outdated Show resolved Hide resolved
* Where `TCtx` is now used, also use `func` instead of `proc`, if applicable

Not all usages of `PCtx` are replaced. `PCtx` is still used in procedures
that form the border between non-VM/VM code. `vmops` also still uses
 `PCtx`, as some ops need to capture a reference to the context
@zerbina
Copy link
Collaborator Author

zerbina commented Mar 14, 2022

Addressed review. I used proc again for the functions that fail the strict side-effect test.

I also fixed the failure of the tcompilerapi.nim test. It failed due to PCtx still being used as arguments in nimeval.nim.

Interestingly enough, in the CI, the test failure was reported to be due to the compiler crashing with a nil access. I ran the test on my machine (Windows) and it also failed with a compiler crash. But, instead of an invalid memory access, I got an assertion raised in semcall.nim.

The compiler crash only happened when importing nimeval. Adjusting the PCtx usage made the crash go away too.

@saem
Copy link
Collaborator

saem commented Mar 14, 2022

I did a quick search in finding usage in semdata and semmagic, but those are at the boundary as mentioned by the PR.

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

And so begins the revamp of the VM in devel. 🎉

Bors r+

@bors
Copy link
Contributor

bors bot commented Mar 14, 2022

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@alaviss
Copy link
Contributor

alaviss commented Mar 14, 2022

Weird, let's try again

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 14, 2022

Build succeeded:

@bors bors bot merged commit ce383f5 into nim-works:devel Mar 14, 2022
@zerbina zerbina deleted the vm-tctx-change branch March 15, 2022 15:46
@haxscramper haxscramper added this to the VM backend refactoring milestone Nov 21, 2022
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.

4 participants