WIP: add proc `cleanAppendInPlace`

[RickBarretto]

Description

• This functions must to modify InPlaced.a
• This must to don't return a new block, just do the above

"In a few words: in-place modification" - @drkameleon

from Discord

Fixes #756

Type of change

• [x] Internal code function
  • [x] It's proposes to improve internal codebase

[drkameleon]

A few comments (mostly Nim-related, rather than specific for Arturo):

let buffer = s

You don't really need to assign s to anything; you can directly work on s (so, s = is perfect, since s is a var parameter).

In Nim, when you initialize an array like newSeq[Value](number), you have to add the elements via an index, e.g. s[0] = .... If the length ends up being less than the initially allocated, then you use .setLen. if you don't initialize the array, then you add elements via .add (and it handles .setLen itself).

I'm not saying you did anything wrong. It's totally correct. I'm just saying... ;)

3. Instead of this:

for i in buffer:
        when not defined(NOERRORLINES):
            if i.kind != Newline:

... you can use the new iterator cleanBlockValues from above (there is another version, not needing the length), like:

for i in cleanBlockValues(s):
    ...

And, I think it's great! :)

[RickBarretto]

So, I got some doubts about it.

1. let s and var s: First, if I use:

proc some_function(var s) =
    let s = s
    for i in s:
       ...

Is the s in for loop the let s or var s?

2. cleanBlockValues usage

It's doing the same thing as for loop's code. What do I must to put inside the for i in cleanedBlockValues(s):?

[drkameleon]

So, I got some doubts about it.

1. let s and var s: First, if I use:

proc some_function(var s) =
    let s = s
    for i in s:
       ...

Is the s in for loop the let s or var s?

First off, we have let and var when declaring variables: let means the variable won't change (so, it's kind of a local constant), var means the variable may change.

That's one thing.

Another thing is about the parameters/arguments of a function: if you have an argument declared like (s: string), it's passed by value; so we can do something with the value but not to the value. if you want to pass by reference, we'd do (s: var string).

In this last case, which is our case, we can change/use the variable directly.

So... in a few words:

This:

proc some_function(var s) =
    let s = s
    for i in s:
       ...

Should be like this:

(removing let s = s altogether)

proc some_function(var s) =
    for i in s:
       ...

2. cleanBlockValues usage

It's doing the same thing as for loop's code. What do I must to put inside the for i in cleanedBlockValues(s):?

cleanedBlockValues is defined here: https://github.com/arturo-lang/arturo/blob/master/src/vm/values/clean.nim#L50-L61

It's basically an iterator that helps us to avoid all this when/if thing: all it returns are values that we can safely use.

So... to answer your question:

This:

    for i in buffer:
        when not defined(NOERRORLINES):
            if i.kind != Newline:
                s[cnt] = i
                cnt += 1
        else:
            s[cnt] = i
            cnt += 1

May become:

    for i in cleanedBlockValues(s):
        s[cnt] = i
        cnt += 1

[RickBarretto]

Hummmm, got it! Thank you! I'll change this code! :wink:

[drkameleon]

Also, another idea:

Since the point is to change the internal array itself, so there's really no need to create another seq (via newSeq). We already have it. So, why not... "stretch" it:

    cleanBlock(s)

    let L1 = len(s.a)
    let L2 = len(t.a)

    s.a.setLen(L1 + L2)
    var cnt = 0
    ....

And then... we just add the 2nd arrays elements beginning at L1.

[drkameleon]

And - yet another comment (sorry for... bombarding you :) ):

cleanAppendInPlace is meant to work on Value objects (and then access the ValueArray at .a)

So the function should be like:

func cleanAppendInPlace*(s: var Value, t: Value) {.inline,enforceNoRaises.} =
    ....

[RickBarretto]

Do you think it should work?

proc cleanAppendInPlace*(s: var Value, t: Value) {.inline,enforceNoRaises.} =
    var cnt = 0
    
    # I didn't set s.a.LenSet here, 
    # because `s`'s length can be the same or greater than cleanedBlocks...
    for i in cleanedBlockValues(s):
        s.a[cnt] = i
        cnt += 1

    # Here I set a new length to s, 
    # because, we'll need its size fit `t`
    # Now the length are (uncleaned s.a + uncleaned t.a)
    s.a.setLen(len(s.a) + len(t.a))

    # Adding cleaned `t`'s values to `s`
    for i in cleanedBlockValues(t):
        s.a[cnt] = i
        cnt += 1

    # cutting the `s`'s tail
    setLen(s.a, cnt)

[RickBarretto]

And - yet another comment (sorry for... bombarding you :) ):

No problem! 😄

cleanAppendInPlace is meant to work on Value objects (and then access the ValueArray at .a)

So the function should be like:

func cleanAppendInPlace*(s: var Value, t: Value) {.inline,enforceNoRaises.} =
    ....

Okay, I'll fix it!

[drkameleon]

(I ended up editing your comment by mistake! lol)

Do you think it should work?

proc cleanAppendInPlace*(s: var Value, t: Value) {.inline,enforceNoRaises.} =
    var cnt = 0
    
    # I didn't set s.a.LenSet here, 
    # because `s`'s length can be the same or greater than cleanedBlocks...
    for i in cleanedBlockValues(s):
        s.a[cnt] = i
        cnt += 1

    # Here I set a new length to s, 
    # because, we'll need its size fit `t`
    # Now the length are (uncleaned s.a + uncleaned t.a)
    s.a.setLen(len(s.a) + len(t.a))

    # Adding cleaned `t`'s values to `s`
    for i in cleanedBlockValues(t):
        s.a[cnt] = i
        cnt += 1

    # cutting the `s`'s tail
    setLen(s.a, cnt)

With a couple of changes:

# we convert `s` and `t` to Values
# since `cleanedBlockValues` needs Values to work on
proc cleanAppendInPlace*(s: var Value, t: Value) {.inline,enforceNoRaises.} =
    # let's clean the s value in-place
    # if it's already clean nothing happens
    cleanBlock(s)

    # let's store the lengths of the two arrays
    let L1 = len(s.a)
    let L2 = len(t.a)

    # Here I set a new length to s, 
    # because, we'll need its size fit `t`
    # Now the length are (uncleaned s + uncleaned t)
    s.a.setLen(L1 + L2)

    # let's set `cnt` where we are left, to `s` current length
    var cnt = L1

    # Adding cleaned `t`'s values to `s.a`
    for i in cleanedBlockValues(t, L2):
        s.a[cnt] = i
        cnt += 1

    # cutting the `s.a`'s tail
    setLen(s.a, cnt)

I think this will work fine.

Remember: s is the Value (which - as in arturo - could be anything, an integer, a string, or a block). Since, in that case, we already know it's a block (that is: holding an array of other values), we may freely access its .a field, which is exactly what we'll be changing. Now, why do we need to pass the Value's instead of the arrays inside? Because there is this extra .dirty field, which allows us to know whether the internal array needs "cleaning" or not. 😉

[RickBarretto]

(I ended up editing your comment by mistake! lol)

I see that, but no problem. It seems clear to me. I'll do a commit, so! 😎

[drkameleon]

I'm seeing a compilation error:

Error: attempting to call undeclared routine: 'keepIf'

And you must be wondering where this keepIf comes from... Well, it's part of cleanBlock (https://github.com/arturo-lang/arturo/blob/master/src/vm/values/clean.nim#L24) and since cleanBlock is a template, it's as if its contents are copy-pasted in there... so, now it's missing keepIf.

All you have to do is add sequtils (the module where keepIf resides: https://nim-lang.org/docs/sequtils.html#keepIf%2Cseq%5BT%5D%2Cproc%28T%29) in the imports at the top of the file, along with the other imports.

[RickBarretto]

@drkameleon the CI is breaking.

Broken code is here: vm/values/clean.nim

CI is giving it:

/Users/runner/work/arturo/arturo/src/vm/vm.nim(70, 11) template/generic instantiation of `importLib` from here
/Users/runner/work/arturo/arturo/src/helpers/arrays.nim(207, 15) template/generic instantiation of `cleanBlock` from here
/Users/runner/work/arturo/arturo/src/vm/values/clean.nim(24, 16) Error: attempting to call undeclared routine: 'keepIf'
  CRASHED!!!
Error: Process completed with exit code 1.

My VsCodium is giving me this:

[RickBarretto]

@drkameleon CI is passing now! 😄

[drkameleon]

And...

...ready to merge! 🚀