Skip to content

Improved handling of value type containers in foreach - #94

Merged
anjo76 merged 2 commits into
anjo76:masterfrom
suprstefan:master
Sep 21, 2026
Merged

anjo76 merged 2 commits into
anjo76:masterfrom
suprstefan:master

Conversation

@suprstefan

Copy link
Copy Markdown
Contributor

I spent some time digging into fixing #92. Since I haven't looked that much at the VM and bytecode for angelscript I might be missing something, but it fixes my case and doesn't break any other at least. And the VM looks pretty standard tbh.

To summarize it and my assumptions:

  • Reference types are stored as pointers internally as far as the VM is concerned.
  • The foreach expressions evaluate the expression to the right of ":" and stores the result in a temporary variable.
  • The temporary variable will be a pointer for reference types and the actual value for value types.
  • When making the calls to opForX the previous behavior always used asBC_PshVPtr which resulted in a by-value copy of what was in the temporary variable storage. This works great for reference types, since it just copies the pointer, but for value types it creates a garbage "this" pointer.
  • To fix this I changed it to instead use asBC_PSF if the container type is a value type, to make it store a pointer to the value as "this".
  • Since references can't be used as lvalues I don't think I have to care about them? I assume they're stored as pointers as well.

Does this look correct?

@anjo76

anjo76 commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Thanks a lot for providing this fix. It will save me time to investigate on my own.

I'll do a more detailed review and testing on my own before I merge it, but at a quick glance on what you've done it does look to be correct.

@anjo76 anjo76 added the bug Something isn't working label Sep 9, 2026
@anjo76 anjo76 self-assigned this Sep 9, 2026
@anjo76
anjo76 merged commit e522fbb into anjo76:master Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants