feat(stdlib): Add lookupCI for assocarray and Node #639

Vasya-M · 2021-04-08T13:47:59Z

solution $629 issue
what were done:

fixed CreateObject("roSGNode", "node")
added support for lookupCI for AA and Nodes
added UT and e2e tests

resolves #629

UPD:

Fixed logic of using/saving keys in AA (saving last key used by [], or lowercase key if accessed by .)

lkipke

I think the lookupCI code looks great, but let's pull out the case-sensitive createObject code into its own PR 🙂

src/brsTypes/components/ComponentFactory.ts

This reverts commit e8ea91d.

lkipke

This looks great, thanks for the updates!

alimnios72

overall looks good, a few comments below

src/brsTypes/components/RoAssociativeArray.ts

src/brsTypes/components/RoSGNode.ts

test/brsTypes/components/RoAssociativeArray.test.js

…h creating fields on Node by []. Fixed logic of using/saving keys in AA

alimnios72 · 2021-04-09T19:05:36Z

src/brsTypes/components/RoSGNode.ts

-                field = new Field(value, fieldType, alwaysNotify);
-                this.fields.set(mapKey, field);
-            }
+            // TODO: change to using interpreter.stderr


If having this creates issues int the current PR (which I think it's breaking unit tests right now) I'm ok with opening a github issue and tackling this work in a separate PR

yep, did it

alimnios72

A few last comments below

alimnios72 · 2021-04-14T17:13:48Z

src/brsTypes/components/BrsComponent.ts

     * @returns the element at `index` if one exists, otherwise throws an Error.
     */
-    get(index: BrsType): BrsType;
+    get(index: BrsType, isCaseSensitiveGet?: boolean): BrsType;


I think you can remove Get and Set from the end of this variables as this can be inferred from the name of the function

src/brsTypes/components/RoAssociativeArray.ts

test/brsTypes/components/RoAssociativeArray.test.js

alimnios72 · 2021-04-14T17:33:45Z

src/brsTypes/components/RoAssociativeArray.ts

@@ -77,7 +76,7 @@ export class RoAssociativeArray extends BrsComponent implements BrsValue, BrsIte
            .map((value: BrsType) => value);
    }

-    get(index: BrsType) {
+    get(index: BrsType, isCaseSensitiveGet = false) {


same suggestion for the name of the variables here and in line 102

alimnios72 · 2021-04-14T19:18:57Z

src/brsTypes/components/RoAssociativeArray.ts

-        let indexValue = this.modeCaseSensitive ? index.value : index.value.toLowerCase();
-        return this.elements.get(indexValue) || this.getMethod(index.value) || BrsInvalid.Instance;
+        return (
+            this.findElement(index.value, isCaseSensitiveGet) ||


I think you can get rid of findElement function and just do the following:

this.elements.get(this.findElementKey(index.value), isCaseSensitiveGet)

because if the key is undefined the map should return that as well

looks like it can not accept undefined

sjbarag

This is a good start, @Vasya-M! I'm a bit worried by the O(n) lookups this introduces in case-insensitive mode, so let's see what we can do to keep that from being an exhaustive search 😃

sjbarag · 2021-04-14T21:08:59Z

src/brsTypes/components/RoAssociativeArray.ts

+        for (let key of this.elements.keys()) {
+            if ((useCaseSensitiveSearch ? key : key.toLowerCase()) === elementKey) {
+                realElementKey = key;
+                break;
+            }
+        }


This seems less than ideal, as it'll cause an O(n) lookup for every access once setCaseInsensitive is called. I'm curious if there's an alternative solution we can take that won't require us to iterate across every element every time. Could we maintain a case-insensitive Map next to the case-sensitive one, and write to both for every set()? It'd of course require twice as much memory, but would maintain constant time lookups.

Hey, I even forget about it, a great catch.
So I've taken your idea of using a second map and used it for mapping lower case keys to case sensitive keys. so it will not consume so much memory, will not introduce additional computation and almost saves previous performance.

but there is still be O(n) when we delete keys in case sensitive mode, cuz we can delete key that was used as alias in KeysMap

sjbarag · 2021-04-14T21:11:22Z

src/brsTypes/components/RoAssociativeArray.ts

+            returns: ValueKind.Dynamic,
+        },
+        impl: (interpreter: Interpreter, key: BrsString) => {
+            let lKey = key.value;


Perhaps I've missed something — why's this variable lKey while the rest are key?

idk why, but it was as is before my changes

lkipke

Overall, looks great! Thanks for all the hard work here!

src/brsTypes/components/RoAssociativeArray.ts

test/e2e/resources/components/roAssociativeArray.brs

alimnios72

Other than the already given feedback this looks good to me

sjbarag

We're really close! I've got one last idea to avoid an O(n) operation for every .delete() call, but otherwise this is looking really good :) Thanks for sticking with it, @Vasya-M!

sjbarag · 2021-04-20T16:26:52Z

src/brsTypes/components/RoAssociativeArray.ts

+                // if we have {"DD": 0, "dD": 0},  keyMap["dd"] is pointed to "DD",
+                // and we delete it, then we should find another value for case insensitive accessing ("dD")


Thanks for calling this out! This is an interesting case, and feels like keyMap should be Map<string, Set<string>>: a map of lowercase keys to all encountered cases of that key. In your example with { "DD": 0, "dD": 1}, keyMap.get("dd") would be the set ("DD", "dD"). When someAA.delete("DD") is called from BrightScript, this function removes the first element from that set (something like let newSet = new Set([ ...keyMap.get(lKey) ].slice(1));) in case-sensitive mode, and removes the entire entry (this.keyMap.delete(lKey)) in case-insensitive mode.

Lookups become pretty similar, using the element at index zero of this Set<string>.

sjbarag

Looks great! Thanks for the changes @Vasya-M 😃

Vasyl Martynych added 2 commits April 8, 2021 16:32

fix issue sjbarag#638, allow "node" node type of "roSGNode"

e8ea91d

add lookupCI for AA and Nodes

e148628

lkipke requested changes Apr 8, 2021

View reviewed changes

src/brsTypes/components/ComponentFactory.ts Outdated Show resolved Hide resolved

lkipke requested review from sjbarag and alimnios72 April 8, 2021 14:14

lkipke added e2e Affects this project's end-to-end test cases (the BrightScript sample files executed during testing) enhancement Any addition to this project's existing capabilities stdlib Affects the standard library included with this BrightScript implementation labels Apr 8, 2021

Revert "fix issue sjbarag#638, allow "node" node type of "roSGNode""

44c6f53

This reverts commit e8ea91d.

Vasya-M requested a review from lkipke April 8, 2021 14:42

lkipke approved these changes Apr 8, 2021

View reviewed changes

alimnios72 reviewed Apr 8, 2021

View reviewed changes

src/brsTypes/components/RoAssociativeArray.ts Show resolved Hide resolved

src/brsTypes/components/RoSGNode.ts Show resolved Hide resolved

test/brsTypes/components/RoAssociativeArray.test.js Show resolved Hide resolved

Vasyl Martynych added 2 commits April 9, 2021 21:23

added comments, moved lookupCI test in separate suit. Fixed bug wit…

29809ef

…h creating fields on Node by []. Fixed logic of using/saving keys in AA

fix lint

393c24e

Vasya-M requested a review from alimnios72 April 9, 2021 18:29

alimnios72 reviewed Apr 9, 2021

View reviewed changes

Vasyl Martynych added 2 commits April 9, 2021 22:15

revert fix for node and creating fild by []

f10202c

fix ut fails

d8f2e58

Vasya-M requested a review from alimnios72 April 9, 2021 19:36

alimnios72 reviewed Apr 14, 2021

View reviewed changes

resolve nits

f353bdf

sjbarag requested changes Apr 14, 2021

View reviewed changes

lkipke self-requested a review April 14, 2021 23:21

Vasyl Martynych added 2 commits April 17, 2021 01:25

fix O(N) computation coplex for AA data accessing.

29ea65a

refactoring for AA's delete method

f7d7bba

Vasya-M requested a review from sjbarag April 16, 2021 22:35

lkipke approved these changes Apr 19, 2021

View reviewed changes

resolve PR comments

d8aade1

Vasya-M requested a review from alimnios72 April 19, 2021 22:36

fix UT

53bed83

alimnios72 approved these changes Apr 19, 2021

View reviewed changes

sjbarag requested changes Apr 20, 2021

View reviewed changes

Vasyl Martynych added 2 commits April 21, 2021 14:33

refactoring keyMap to store set of keys

f0a56a7

fix AA UT

020bc4a

Vasya-M requested a review from sjbarag April 21, 2021 11:54

sjbarag approved these changes Apr 30, 2021

View reviewed changes

sjbarag changed the title ~~Issue 629 add lookupCI method~~ feat(stdlib): Add lookupCI for assocarray and Node Apr 30, 2021

sjbarag merged commit 08baf5e into sjbarag:main Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stdlib): Add lookupCI for assocarray and Node #639

feat(stdlib): Add lookupCI for assocarray and Node #639

Vasya-M commented Apr 8, 2021 •

edited

Loading

lkipke left a comment

lkipke left a comment •

edited

Loading

alimnios72 left a comment

alimnios72 Apr 9, 2021

Vasya-M Apr 9, 2021

alimnios72 left a comment

alimnios72 Apr 14, 2021

Vasya-M Apr 14, 2021

alimnios72 Apr 14, 2021

Vasya-M Apr 14, 2021

alimnios72 Apr 14, 2021

Vasya-M Apr 14, 2021 •

edited

Loading

Vasya-M Apr 14, 2021

sjbarag left a comment

sjbarag Apr 14, 2021

Vasya-M Apr 16, 2021

sjbarag Apr 14, 2021

Vasya-M Apr 16, 2021

Vasya-M Apr 19, 2021

lkipke left a comment

alimnios72 left a comment

sjbarag left a comment

sjbarag Apr 20, 2021

Vasya-M Apr 21, 2021

sjbarag left a comment

		// if we have {"DD": 0, "dD": 0}, keyMap["dd"] is pointed to "DD",
		// and we delete it, then we should find another value for case insensitive accessing ("dD")

feat(stdlib): Add lookupCI for assocarray and Node #639

feat(stdlib): Add lookupCI for assocarray and Node #639

Conversation

Vasya-M commented Apr 8, 2021 • edited Loading

lkipke left a comment

Choose a reason for hiding this comment

lkipke left a comment • edited Loading

Choose a reason for hiding this comment

alimnios72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimnios72 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

Vasya-M Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjbarag 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

lkipke left a comment

Choose a reason for hiding this comment

alimnios72 left a comment

Choose a reason for hiding this comment

sjbarag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjbarag left a comment

Choose a reason for hiding this comment

Vasya-M commented Apr 8, 2021 •

edited

Loading

lkipke left a comment •

edited

Loading

Vasya-M Apr 14, 2021 •

edited

Loading