-
Notifications
You must be signed in to change notification settings - Fork 0
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
Variable allocations #7
Conversation
Can @segaljared, @cglzaguilar, @spathiwa please take a look to this? |
source/core/CEntryPoints.cpp
Outdated
} | ||
|
||
if (dataRef == nullptr) { | ||
return kEggShellResult_NullDataPointer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead use kEggShellResult_InvalidDataPointer, that seems to be more generic and useful in other situations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like it better.
|
||
*dataRefLocation = nullptr; | ||
Int32 topSize = typeRef->TopAQSize(); | ||
void* pData = THREAD_TADM()->Malloc(topSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember Paul recommending to not use Malloc in Vireo in general. Is there a function in tm (TypeManagerRef) that will allocate memory for us given a TypeRef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test to ensure that we are not leaking memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, THREAD_TADM() is already using tm under the hood.
source/core/CEntryPoints.cpp
Outdated
return kEggShellResult_NullDataPointer; | ||
} | ||
|
||
NIError error = typeRef->ClearData(dataRef); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a free call to release the dataRef pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll add that. I couldn't find an easy way to enable memory leak checking from JS.
I'll create a new PR basing on master with the addressed feedback. |
This PR is intended for review purposes only. I will create a new PR after addressing feedback gathered here and ni#471 is merged into master.
Exposing 2 functions that will allow JS to allocate variables based on a Type declared in the .via code and of course deallocate the memory.
The workflow goes as follow:
You can take a look to
AllocateTypes.Test.js
too.