Vireo EggShell Refactor for Supporting More Types #471

rajsite · 2018-06-28T23:29:10Z

Implementation of "Vireo API Changes for Supporting More Types Design Doc" - NITalk - PDF

This change encapsulates the breaking API changes associated with the Vireo API refactor. There is still additional work to be done out of master after the change is merged consisting of:

Making error checks and error handling consistent across all of the new api functions: JS API Refactor Acceptance Conditions #466
Migrating the following JavaScript users of the now deprecated API functions to the new API functions: HttpClient, JavaScriptInvoke, Core (FPSync, LV Error Handling), Events.
Removing the deprecated API functionality

LVjs Build: Still in-progress testing changes in Bajor/Main

rajsite · 2018-07-02T17:20:13Z

source/core/module_typeHelpers.js

+            var i = 0,
+                typeHandler;
+
+            for (i = 0; typeHandlers.length; i += 1) {


Should be i < typeHandlers.length

😮 you are right. dunno how that slipped through. I'll fix it later today.

Was this code exercised by one of the tests?

Seems like it couldn't have, it would be an infinite loop.

Yeah, we are missing a test for a type that we cannot handle. I'll add that as well as fixing this.

rajsite · 2018-07-02T21:24:17Z

source/core/module_typeHelpers.js

+
+        var dispatchVisitCluster = function (typeVisitor, valueRef, data) {
+            validateVisitMethod(typeVisitor.visitCluster, 'visitCluster');
+            return typeVisitor.visitCluster(typeVisitor, valueRef, data);


Should be typeVisitor.visitCluster(valueRef, data);

Yes, I'lll fix it

sanmut

Still reviewing

sanmut · 2018-07-03T04:07:35Z

source/core/CEntryPoints.cpp

 }
 //------------------------------------------------------------
 //! Read a symbol's value as a string. Value will be formatted according to the format designated.
-VIREO_EXPORT const char* EggShell_ReadValueString(TypeManagerRef tm,
-        const char* viName, const char* eltName, const char* format)
+VIREO_EXPORT EggShellResult EggShell_ReadValueString(TypeManagerRef tm, const TypeRef typeRef, void* data, const char* format, UInt8** valueString)


Why not keep valueString char **?

Begin() returns a UInt8*. Might as well avoid the cast.

sanmut · 2018-07-03T04:20:08Z

source/core/CEntryPoints.cpp

-//! Returns -1 if the symbols is not found, -2 if was not possible to resize the array and 0 if resizing was successful.
-VIREO_EXPORT Int32 EggShell_ResizeArray(TypeManagerRef tm, const char* viName, const char* eltName, Int32 rank, Int32* newLengths)
+VIREO_EXPORT EggShellResult EggShell_ResizeArray(TypeManagerRef tm, const TypeRef typeRef, const void* pData,
+                                                 Int32 newDimensionsLength, Int32 newDimensions[])


newDimensionsLength is quite a misleading name for rank. We should stick to using rank. If newLengths is not clear enough, that can be renamed as newDimensionLengths.

I'm down for renaming the newDimensions[] input to dimensionLengths[] as that is the name used in the rest of vireo.

How do y'all feel about me removing the newDimensionsLength/Rank input?

All it does is take the passed value and check that it is equal to the Rank Vireo already knows.

I would rather add a comment that says dimensionLengths[] should have a number of items equal to the Rank of the array being manipulated. It seems like an unnecessary input since this api is not allowing changing the rank of a Vireo array.

Actually, renaming newDimensions -> dimensionLengths

and

newDimensionsLength -> rank

is fine. @gleonoliva might be tackling this in a bit

sanmut · 2018-07-03T04:31:49Z

source/core/CEntryPoints.cpp

+//------------------------------------------------------------
+//! Get the values for dimensions of the array. Assumes dimensions target is of length equal to rank
+//! Caller is expected to allocate an array dimensions of size array rank for the duration of function invocation.
+VIREO_EXPORT void Data_GetArrayDimensions(const void* pData, IntIndex dimensions[])


"Dimensions" as a name by itself is ambiguous. What about Dimensions? Number of Dimensions or Length of each dimension? I would be consistent with Vireo's terminology: Use Rank for number of Dimensions which is an integer; use DimensionLengths for lengths of each dimension which is an integer array.
e.g.:
Data_GetArrayDimensionLengths(...., Int dimensionLengthts[])

Will change to the dimensionLengths name used in the rest of Vireo

sanmut · 2018-07-03T04:35:26Z

source/core/CEntryPoints.cpp

+}
+//------------------------------------------------------------
+//! Get the total length for an array
+VIREO_EXPORT Int32 Data_GetArrayLength(const void* pData)


This name ArrayLength is good 👍

sanmut · 2018-07-03T04:53:09Z

source/core/CEntryPoints.cpp

+        STACK_VAR(String, tempReturn);
+        returnBuffer = tempReturn.DetachValue();
+    } else {
+        returnBuffer->Resize1D(0);


Resize1D(0);

Instead of resizing it to a size of 0 and then appending immediately, Resize1D can be called with name.Length()+1 and then name can be copied to instead of getting appended and will be more efficient.

Also, in other places in this PR.

sanmut · 2018-07-03T04:57:58Z

source/core/CEntryPoints.cpp

+
+    if (returnBuffer) {
+        returnBuffer->AppendSubString(&name);
+        // Add an explicit nullptr terminator so it looks like a C string.


nullptr terminator --> null terminator

When applying this change, don't forget to update other functions with the same comment.

Yes I took care of that

sanmut · 2018-07-03T05:05:27Z

source/core/TypeAndDataManager.cpp

+    TypeRef t = this;
+    while (t) {
+        SubString typeName = t->Name();
+        SubString analogTypess(TypeAnalogWaveform);


SubString analogTypess(TypeAnalogWaveform);

this temporary is misspelled but not likely even needed.

I'll change the name

sanmut · 2018-07-03T15:46:47Z

source/core/CEntryPoints.cpp

@@ -546,6 +651,66 @@ VIREO_EXPORT TypeRef TypeRef_GetSubElementByIndex(TypeRef typeRef, Int32 index)
    return typeRef->GetSubElement(index);
 }
 //------------------------------------------------------------
+VIREO_EXPORT Boolean TypeRef_IsCluster(TypeRef typeRef)


Should we have the prefix of EggShell for all the new APIs that start with TypeRef as well?

Since we are not exposing these functions to eggShell, I don't think they should be prefixed as eggshell. That's why we added the typeHelpers module.

sanmut · 2018-07-03T18:28:52Z

source/io/module_eggShell.js

+            return valueRef;
+        };
+
+        Module.eggShell.findSubValueRef = publicAPI.eggShell.findSubValueRef = function (valueRef, path) {


path should be renamed to subPath to reflect it is not the full path.

I'll change it.

spathiwa

Excellent work. This was an ambitious refactor.

spathiwa · 2018-07-03T19:13:49Z

test-it/ViaTests/ArrayDemo.via

@@ -10,6 +10,7 @@ define(ArrayDemo dv(.VirtualInstrument (

    e(a(.Int32 * *)   variableArray2d)
    e(a(.Int32 2 3)   fixedArray2d)
+    e(a(.Int32 0 0)   fixedArray2dEmpty)


Maybe add comments to via tests with items that appear to be unused like this one to note that they are read programmatically by a karma test.

mehagar · 2018-07-09T20:28:55Z

source/include/CEntryPoints.h

@@ -21,6 +21,9 @@ typedef enum {
    kEggShellResult_UnexpectedObjectType = 2,
    kEggShellResult_InvalidResultPointer = 3,
    kEggShellResult_UnableToCreateReturnBuffer = 4,
+    kEggShellResult_InvalidTypeRef = 5,
+    kEggShellResult_MismatchedArrayRank = 6,
+    kEggSehllResult_UnableToParseData = 7,


Thanks for catching this one. I'll fix it 👍

mehagar · 2018-07-09T20:30:26Z

source/core/module_typeHelpers.js

+
+        Module.typeHelpers.findTypeDispatcher = function (typeRef) {
+            var i = 0,
+                typeHandler;


spacing issue

What's the spacing issue? Linter did not complain with this.

cglzaguilar

I have some comments so far.

cglzaguilar · 2018-07-09T21:21:18Z

source/core/CEntryPoints.cpp

 {
-    void *pData = nullptr;
-
    SubString objectName(viName);


Add TypeManagerScope (tm), before this line.

You are right, I'll change it.

cglzaguilar · 2018-07-09T21:37:11Z

source/core/CEntryPoints.cpp

+}
+//------------------------------------------------------------
+// Write a string value to a symbol. Value will be parsed according to format designated.
+VIREO_EXPORT EggShellResult EggShell_WriteValueString(TypeManagerRef tm, const TypeRef typeRef, void* data, const char* format, const char* value)


In this and other functions use pData instead of data to keep the naming consistent.

I'll fix it.

cglzaguilar · 2018-07-09T21:39:43Z

source/core/CEntryPoints.cpp

+    return kEggShellResult_Success;
+}
+//------------------------------------------------------------
+// Write a string value to a symbol. Value will be parsed according to format designated.


"to designated format" instead of "to format designated"?

I'll change it

cglzaguilar · 2018-07-09T21:51:57Z

source/core/CEntryPoints.cpp

+
+    if (returnBuffer) {
+        returnBuffer->AppendSubString(&name);
+        // Add an explicit nullptr terminator so it looks like a C string.


When applying this change, don't forget to update other functions with the same comment.

Also addressed feedback from the PR.

…ions

Also added a test for unssported Enums of 64 bits.

Standardized on call for dispatchers and fixed loop condition.

Timestamps are 2 UInt64 therefore their JSON representation are strings. Updating expected values to match this.

Also added tests that verify this behavior.

fix for findSubValueRef. I added tests that effectively fail before the fix and pass after making it. Included is the renaming of TypeWaveform.test.js to TypeWaveform.Test.js which were not being executed because of the regex used in karma.conf to include tests. I also fixed those.

* Adressed feedback from PR * Renamed subpathstackPointer * Fixing typo, variable names and TypeManagerScope for FindValue. * Renamed pData in header

rajsite · 2018-07-10T00:16:21Z

@gleonoliva rebased on top of master at 581798e

cglzaguilar

Please address minor feedback, other than that, good to go from my part.

cglzaguilar · 2018-07-09T22:00:47Z

source/core/TypeAndDataManager.cpp

+{
+    TypeRef t = this;
+    while (t) {
+        if (t->Name().Compare(&TypeInt8) || t->Name().Compare(&TypeInt16) || t->Name().Compare(&TypeInt32)


Minor comment for readability, keep all signed checks on one line and the unsigned checks in another line.

cglzaguilar · 2018-07-10T18:52:45Z

test-it/karma/fixtures/publicapi/ReadWriteErrorCluster.via

+)))
+
+enqueue (MyVI)
+// Finished!


Add a new line at the end of the file to remove the "red marker"

cglzaguilar · 2018-07-10T18:55:48Z

test-it/karma/publicapi/Double.Test.js

+            expect(readDouble('enum64releases')).toBe(5);
+        });
+
+        it('to read different boolean types from memory', function () {


Change "boolean types" to "boolean values"

All other tests use the word 'types' in them do you want me to change those too?

Oh I see the inconsistency, I'll change all of them to be values.

cglzaguilar · 2018-07-10T18:57:23Z

test-it/karma/publicapi/Double.Test.js

+            testWriteDouble('enum64releases', 5, 4);
+        });
+
+        it('to write different boolean types to memory', function () {


Same thing here, "boolean types" to "boolean values".

cglzaguilar · 2018-07-10T18:59:41Z

test-it/karma/publicapi/DoubleInAggregate.Test.js

+            expect(readDouble('dataItem_ArrayOfTimestamp', '0')).toMatchIEEE754Number(3564057536.423476);
+        });
+
+        it('to read different double values from N-dimensional arrays', function () {


Please add a 3D case on this list.

There is a 3D case on line 41

* Changed test names to from types to values to keep them consistent * Added an extra line at the end to remove red marker * Improving readibility by having signed type checks in same line

Use decoded field names in readValueRefObject

rajsite requested review from cglzaguilar, sanmut and spathiwa as code owners June 28, 2018 23:29

rajsite requested review from gleono and segaljared June 28, 2018 23:29

rajsite force-pushed the api-type-refactor-rebase branch from 8cca327 to 2ef6124 Compare June 29, 2018 23:35

rajsite commented Jul 2, 2018

View reviewed changes

sanmut reviewed Jul 3, 2018

View reviewed changes

spathiwa approved these changes Jul 3, 2018

View reviewed changes

segaljared approved these changes Jul 4, 2018

View reviewed changes

mehagar reviewed Jul 9, 2018

View reviewed changes

cglzaguilar reviewed Jul 9, 2018

View reviewed changes

gleono approved these changes Jul 9, 2018

View reviewed changes

rajsite and others added 14 commits July 9, 2018 19:04

Add the findValueRef API function

167e818

Update readDouble for ValueRef api

c4fbfd7

Add the shared createValueRef function

493d545

Fix usage of createValueRef

7fa3d60

Test unsupported types for readDouble

ce1727c

Merged changes for readDouble

e69710c

Exported more Is* functions from TypeCommon

20c171c

Merged isType functions

56d5567

Added tests for reflectOnValueRef and fixed some issues.

f303d55

Fixed whitespace in EmMakeFile and removed cwrap for TopAQSize.

d58340d

Moved all type related functions to its own module.

b7c830f

Also addressed feedback from the PR.

Fixed linting issues

6db6834

Modified TypeRef_Name to take a TypeManagerRef as other similar funct…

5f812dc

…ions

Removed string allocations for errors

cfd90b0

rajsite and others added 19 commits July 9, 2018 19:05

First pass of writeString

a905e11

Update comment for jsStringToSizedUTF8Array

cc225b8

Expand readString and writeString tests

d59084d

First pass writeDouble api

b14bda8

Expand writeDouble tests and use EggShellResult

5eb6d59

Aded visitEnum[8|16|32] to visitor reflection API

4f8a8e2

Changed name of visitor functions for enums

0e9519d

Also added a test for unssported Enums of 64 bits.

Mark deprecated functions in eggShell api

e9e9b21

Standardized on call for dispatchers and fixed loop condition.

7004589

Added tests for context and arguments of visitor functions.

72050a1

Merge pull request #2 from gleonoliva/typehelpers-fixes

f391fd4

Standardized on call for dispatchers and fixed loop condition.

Renamed TypeWaveform tests to be included in the test suite.

ae135ca

Fixed tests in TypeWaveform.

9db209f

Timestamps are 2 UInt64 therefore their JSON representation are strings. Updating expected values to match this.

Fixing FindSubValueRef

e79f1e3

Also added tests that verify this behavior.

Fixed linting errors

c01c7e2

Renamed tests

558ff6f

Reverted writeJSStringToStack and added a free memory on writeJSON.

32b5f73

Type support feedback (#4)

1eef26c

* Adressed feedback from PR * Renamed subpathstackPointer * Fixing typo, variable names and TypeManagerScope for FindValue. * Renamed pData in header

rajsite force-pushed the api-type-refactor-rebase branch from 730a861 to 1eef26c Compare July 10, 2018 00:14

cglzaguilar approved these changes Jul 10, 2018

View reviewed changes

Guillermo Leon and others added 3 commits July 10, 2018 15:24

Type support feedback5 (#5)

7754218

* Changed test names to from types to values to keep them consistent * Added an extra line at the end to remove red marker * Improving readibility by having signed type checks in same line

Use decoded field names in readValueRefObject

8498742

Merge pull request #6 from rajsite/valuerefobject-decoded-names

80c9e0f

Use decoded field names in readValueRefObject

gleono mentioned this pull request Jul 12, 2018

Variable allocations rajsite/VireoSDK#7

Closed

rajsite merged commit 8bdcb79 into ni:master Jul 13, 2018

gleono pushed a commit to gleono/VireoSDK that referenced this pull request Jul 13, 2018

Merge after ni#471 made it to master

73c9f9a

gleono mentioned this pull request Jul 16, 2018

Update PropertyNode JS Callbacks #479

Merged

gleono pushed a commit to gleono/VireoSDK that referenced this pull request Jul 17, 2018

Merge after ni#471 made it to master

5937d7f

Vireo EggShell Refactor for Supporting More Types #471

Vireo EggShell Refactor for Supporting More Types #471

Conversation

rajsite commented Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

gleono Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanmut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsite Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spathiwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cglzaguilar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsite commented Jul 10, 2018

cglzaguilar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsite commented Jun 28, 2018 •

edited

Loading

gleono Jul 2, 2018 •

edited

Loading

rajsite Jul 3, 2018 •

edited

Loading