-
Notifications
You must be signed in to change notification settings - Fork 44
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
Vireo ResizeArray implementation #464
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,6 @@ | |
'EggShell_WriteValueString', | ||
'EggShell_GetPointer', | ||
'EggShell_GetArrayDimLength', | ||
'EggShell_ResizeArray', | ||
'Data_ValidateArrayType', | ||
'Data_GetStringBegin', | ||
'Data_GetStringLength', | ||
|
@@ -85,7 +84,8 @@ | |
UNEXPECTED_OBJECT_TYPE: 2, | ||
INVALID_RESULT_POINTER: 3, | ||
UNABLE_TO_CREATE_RETURN_BUFFER: 4, | ||
INVALID_TYPE_REF: 5 | ||
INVALID_TYPE_REF: 5, | ||
MISMATCHED_ARRAY_RANK: 6 | ||
}; | ||
var eggShellResultEnum = {}; | ||
eggShellResultEnum[EGGSHELL_RESULT.SUCCESS] = 'Success'; | ||
|
@@ -94,6 +94,7 @@ | |
eggShellResultEnum[EGGSHELL_RESULT.INVALID_RESULT_POINTER] = 'InvalidResultPointer'; | ||
eggShellResultEnum[EGGSHELL_RESULT.UNABLE_TO_CREATE_RETURN_BUFFER] = 'UnableToCreateReturnBuffer'; | ||
eggShellResultEnum[EGGSHELL_RESULT.INVALID_TYPE_REF] = 'InvalidTypeRef'; | ||
eggShellResultEnum[EGGSHELL_RESULT.MISMATCHED_ARRAY_RANK] = 'MismatchedArrayRank'; | ||
|
||
// Keep in sync with NIError in DataTypes.h | ||
var niErrorEnum = { | ||
|
@@ -115,7 +116,6 @@ | |
var EggShell_WriteValueString = Module.cwrap('EggShell_WriteValueString', 'void', ['number', 'string', 'string', 'string', 'string']); | ||
var EggShell_GetPointer = Module.cwrap('EggShell_GetPointer', 'number', ['number', 'string', 'string', 'number', 'number']); | ||
var EggShell_GetArrayDimLength = Module.cwrap('EggShell_GetArrayDimLength', 'number', ['number', 'string', 'string', 'number']); | ||
var EggShell_ResizeArray = Module.cwrap('EggShell_ResizeArray', 'number', ['number', 'string', 'string', 'number', 'number']); | ||
var Data_ValidateArrayType = Module.cwrap('Data_ValidateArrayType', 'number', ['number', 'number']); | ||
var Data_GetStringBegin = Module.cwrap('Data_GetStringBegin', 'number', []); | ||
var Data_GetStringLength = Module.cwrap('Data_GetStringLength', 'number', []); | ||
|
@@ -425,8 +425,7 @@ | |
}; | ||
|
||
Module.eggShell.getArrayDimensions = publicAPI.eggShell.getArrayDimensions = function (valueRef) { | ||
var TypedArrayConstructor = findCompatibleTypedArrayConstructor(valueRef.typeRef); | ||
if (TypedArrayConstructor === undefined) { | ||
if (!Module.typeHelpers.isArray(valueRef.typeRef)) { | ||
throw new Error('Performing getArrayDimensions failed for the following reason: ' + eggShellResultEnum[EGGSHELL_RESULT.UNEXPECTED_OBJECT_TYPE] + | ||
' (error code: ' + EGGSHELL_RESULT.UNEXPECTED_OBJECT_TYPE + ')' + | ||
' (typeRef: ' + valueRef.typeRef + ')' + | ||
|
@@ -572,20 +571,30 @@ | |
return EggShell_GetArrayDimLength(Module.eggShell.v_userShell, vi, path, dim); | ||
}; | ||
|
||
Module.eggShell.resizeArray = publicAPI.eggShell.resizeArray = function (vi, path, newDimensionSizes) { | ||
var int32Byte = 4; | ||
var rank = newDimensionSizes.length; | ||
var newLengths = Module._malloc(rank * int32Byte); | ||
|
||
for (var i = 0; i < rank; i += 1) { | ||
Module.setValue(newLengths + (i * int32Byte), newDimensionSizes[i], 'i32'); | ||
Module.eggShell.resizeArray = publicAPI.eggShell.resizeArray = function (valueRef, newDimensions) { | ||
if (!Array.isArray(newDimensions)) { | ||
throw new Error('Expected newDimensions to be an array of dimension lengths, instead got: ' + newDimensions); | ||
} | ||
|
||
var success = EggShell_ResizeArray(Module.eggShell.v_userShell, vi, path, rank, newLengths); | ||
|
||
Module._free(newLengths); | ||
|
||
return success; | ||
var stack = Module.stackSave(); | ||
var newDimensionsLength = newDimensions.length; | ||
var dimensionsPointer = Module.stackAlloc(newDimensionsLength * LENGTH_SIZE); | ||
var i, currentDimension; | ||
for (i = 0; i < newDimensionsLength; i += 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it common practice is JS to use i += 1, instead of i++? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is a linting rule in |
||
currentDimension = newDimensions[i]; | ||
|
||
if (typeof currentDimension !== 'number') { | ||
throw new Error('Expected all dimensions of newDimensions to be numeric values for dimension length, instead got' + currentDimension); | ||
} | ||
Module.setValue(dimensionsPointer + (i * LENGTH_SIZE), currentDimension, 'i32'); | ||
} | ||
var eggShellResult = Module._EggShell_ResizeArray(Module.eggShell.v_userShell, valueRef.typeRef, valueRef.dataRef, newDimensionsLength, dimensionsPointer); | ||
if (eggShellResult !== 0) { | ||
throw new Error('Resizing the array failed for the following reason: ' + eggShellResultEnum[eggShellResult] + | ||
' (error code: ' + eggShellResult + ')' + | ||
' (typeRef: ' + valueRef.typeRef + ')' + | ||
' (dataRef: ' + valueRef.dataRef + ')'); | ||
} | ||
Module.stackRestore(stack); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know if any of the Errors thrown above would cause a memory leak? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Written under the assumption that on exception throw to JS the whole Vireo instance (stack + heap) should be considered corrupt. We should verify that assumption is usable and standardize across functions prior to merge. Tracked as "Robustness of stackSave / restore and error recovery the api should enable" in #466 |
||
}; | ||
|
||
Module.eggShell.dataReadString = function (stringPointer) { | ||
|
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.
Consider const also for TypeManagerRef. Also, add at least null checks for tm. We might need to update other API calls to validate tm.
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.
None of the other functions in CEntryPoints.cpp use const for the TypeManagerRef. Maybe we can submit that as a separate change before merging
ni:api-type-refactor
to master.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.
Made an issue to track things we should verify across all the implemented functions before merging. Tracked as "Consistent marking of parameters as const on the CEntryPoints" in #466