Skip to content
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

add inclusive array operator #148

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tsujp
Copy link
Contributor

@tsujp tsujp commented Jun 27, 2021

  • This PR WITHOUT everything involving inclusive works; however
  • The print output from the REPL from this change is wrong so I attempted to fix this by adding inclusive for the purposes of deciding between ... and .. in the REPL print feedback. This broke everything that worked before, likely because I don't fully understand the VM yet and am managing something. I've submitted it like this for general feedback of "don't worry about the REPL print yet" or "let's try and fix the REPL print".

@tsujp
Copy link
Contributor Author

tsujp commented Jun 27, 2021

Everything worked fine until I touched inclusive now it always makes inclusive lists regardless of .. or .... Committing as-is for review on the implementation of inclusive for the purposes of the REPL print feedback (scrap or try fix).

In other news in pk_vm.c I tried in-lining the range from and to as follows but I kept getting floating point precision errors. That's just a small lines-of-code optimisation, but I'm unsure why it happens since AS_NUM returns a double anyway. I assume there is some difference between double x = AS_NUM(from) versus (double)AS_NUM(from) meaning the latter has a precision error. I understand the latter is a cast and it's probably due to me using this magic in a cast.

PUSH(VAR_OBJ(newRange(vm, (double)AS_NUM(from), (double)AS_NUM(to))));

REPL result:

>>> r = 1..5
[Range:1..5.000000000000001]
>>> r.as_list
[1, 2, 3, 4, 5]
>>> for i in 1..5 do print(i) end
# continues forever, 1, 2, 3, 4, 5 ... 300000... on and on

Indepedent of this there was an 'informational' (for lack of a better term) error too:

>>> r = 1..5
[Range:1..6]
>>> r.as_list
[1, 2, 3, 4, 5]

# this is correct behaviour, nice!

>>> x = 1...6
[Range:1..6]
>>> x.as_list
[1, 2, 3, 4, 5]

# again correct behaviour but output is double-dot and not triple dot and so
# the REPL feedback `[Range:1..6]` is wrong it should be `[Range:1...6]`

Correct me if I am wrong but my adjustments to that display only occur in REPL mode and so wont
slow down running Pocketlang with input from a file, correct?

@ThakeeNathees ThakeeNathees added enhancement Improvements or additions to the source implementation New functionality addition labels Jun 27, 2021
@ThakeeNathees
Copy link
Owner

ThakeeNathees commented Jun 27, 2021

As I've said here #142 (comment), you shouldn't add a boolean inclusive to the range object. And all the range objects are exclusive internally.

We don't have to check if the range is inclusive or exclusive at different places in our code

        char* buf_operator;
        if (range->inclusive) {
          buf_operator = malloc(2);
          buf_operator = "..";
        } else {
          buf_operator = malloc(3);
          buf_operator = "...";
        }

This is exactly what I meant when I say, "to check if the range is inclusive or exclusive at different places in our code"


Our range are always exclusive, but when we're creating one, depend on the syntax we change the limit of the range.

Ex:

0..5  = newRange(vm, 0, 5+1)
0...5 = newRange(vm, 0, 5)

If you find #142 (comment) harder to understand let me know, I'll help you with detailed explanation on how to do it.

@tsujp

@tsujp
Copy link
Contributor Author

tsujp commented Jun 28, 2021

I think there's been some miscommunication here @ThakeeNathees -- I have not implemented the core logic using a bool inclusive; I've reverted (without a force commit) the inclusive stuff to show the bare-implementation.

My questions about inclusive are because the REPL does not change its output. It displays the wrong to and from, and the wrong operator .. vs ... because it is hardcoded so: https://github.com/tsujp/pocketlang/blob/feat/add-inclusive-array-operator/src/pk_var.c#L1425-L1429

How do you want to solve the REPL problem? I don't see a way to have OP_RANGE_IN and OP_RANGE_EX accessible in that area so we need something to determine whether to print .. or ....

@tsujp
Copy link
Contributor Author

tsujp commented Jun 28, 2021

The alternative is we leave the REPL saying 1..6 for a range defined as 1..5 and 1..5 for a range defined as 1...5 but I don't think this is very clear at first glance.

Copy link
Owner

@ThakeeNathees ThakeeNathees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is we leave the REPL saying 1..6 for a range defined as 1..5 and 1..5 for a range defined as 1...5 but I don't think this is very clear at first glance.

Simple. Add a period . to the toString() of range object (because all ranges are exclusive internally)

-        pkByteBufferAddString(buff, vm, "..", 2);
+        pkByteBufferAddString(buff, vm, "...", 3);

This will print like below

>>> 1..5
[Range:1...6]
>>> 1...5
[Range:1...5]

src/pk_compiler.c Outdated Show resolved Hide resolved
src/pk_compiler.c Outdated Show resolved Hide resolved
@tsujp tsujp requested a review from ThakeeNathees July 5, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements or additions to the source implementation New functionality addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants