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

make get for options use lent T #14442

Merged
merged 1 commit into from
May 25, 2020
Merged

make get for options use lent T #14442

merged 1 commit into from
May 25, 2020

Conversation

cooldome
Copy link
Member

@cooldome cooldome commented May 24, 2020

fixes #14420

@timotheecour
Copy link
Member

timotheecour commented May 25, 2020

I'm confused; what about #14417 (comment) ? at very least this needs a test at RT and CT
also the title of #14420 is The VM doesn't support lent annotation, so unless you're fixing that (which doesn't look like it from PR content), I'd remove fixes #14420 from PR msg

@cooldome
Copy link
Member Author

cooldome commented May 25, 2020

@mratsim tried adding lent T to get() function of Option[T] and received error Error: limited VM support for 'addr' and thought that VM has no support for lent T. It turned out that VM has full support of lent T/var T and the issue is caused not by lent T but by temporary variable introduced by vmgen to handle stmtListExpr. One miinor change to get() was needed to avoid stmtListExpr.

Option[T] is already extensively covered by tests.

@mratsim
Copy link
Collaborator

mratsim commented May 25, 2020

Is the temporary introduced by the VM needed?

Otherwise maybe we should add "Try returning via the implicit result" to the error message.

unsafeGet should also be updated in this PR.

@timotheecour
Copy link
Member

timotheecour commented May 25, 2020

We should fix #14420 (comment) before closing that issue, so I still don't think your PR should close that issue as it remains an incompatibility between RT and VM

As a potential impact, I tried to use options in my RayTracer, but they were 5x slower (not just a couple dozen percents like the iterator issue in #14421) than passing mutable parameter + bool
Well nobody claimed Option[T] is a lightweight abstraction.

  • Option[T] is already extensively covered by tests.

there's 0 VM test

@Araq Araq merged commit 6635874 into devel May 25, 2020
@Araq Araq deleted the lent_for_options branch May 25, 2020 13:31
@arnetheduck
Copy link
Contributor

this doesn't fix the issue though, just works around it for Option specifically - ie result = self.val is just a hack, the same construct exists in lots and lots of other code.

EchoPouet pushed a commit to EchoPouet/Nim that referenced this pull request Jun 13, 2020
Co-authored-by: cooldome <ariabushenko@bk.ru>
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.

VM and js backend require result for lent or var annotations to work
5 participants