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

Range: Inclusive and exclusive operators #142

Open
Gundolf68 opened this issue Jun 25, 2021 · 6 comments
Open

Range: Inclusive and exclusive operators #142

Gundolf68 opened this issue Jun 25, 2021 · 6 comments
Labels
feature request Requesting for new featuers TODO It hasn't implemented yet.

Comments

@Gundolf68
Copy link

Since pocketlang is in an early stage: What if pocketlang had two range operators like ruby or wren (".." and "...")?

@Gundolf68 Gundolf68 changed the title Range: Inclusive and exlusive operators Range: Inclusive and exclusive operators Jun 25, 2021
@ThakeeNathees ThakeeNathees added feature request Requesting for new featuers TODO It hasn't implemented yet. labels Jun 26, 2021
@ThakeeNathees
Copy link
Owner

I'm not a fan of inclusive range but, since it's something ruby has, we have to implement it in order to prevent someone confusing with the different behavior (especially one who has a ruby background). I'll add this to the TODO list.

@tsujp
Copy link
Contributor

tsujp commented Jun 26, 2021

I was looking at this too, might take a crack at it.

@tsujp
Copy link
Contributor

tsujp commented Jun 26, 2021

@ThakeeNathees did you want an entirely new object type or did we want an attribute on the exiting OBJ_RANGE type indicating if it's exclusive or inclusive. I think the latter, the attribute, would be a better approach.

Also, part of this would be changing the current 1..5 to match Ruby's. Currently 1..5 is actually the values [1, 2, 3, 4] whereas in Ruby it is [1, 2, 3, 4, 5] and in Ruby the ellipses ... range is what Pocketlang has as the double-dot range ...

@tsujp
Copy link
Contributor

tsujp commented Jun 26, 2021

Just so it's clear:

pocketlang >>> 1..5 # [1, 2, 3, 4]
ruby       >>> 1..5 # [1, 2, 3, 4, 5]
wren       >>> 1..5 # [1, 2, 3, 4, 5]

Change pocketlang .. to match Ruby's, giving:

pocketlang >>> 1..5 # [1, 2, 3, 4, 5]
ruby       >>> 1..5 # [1, 2, 3, 4, 5]
wren       >>> 1..5 # [1, 2, 3, 4, 5]

Add ... giving:

pocketlang >>> 1...5 # [1, 2, 3, 4]
ruby       >>> 1...5 # [1, 2, 3, 4]

@ThakeeNathees
Copy link
Owner

ThakeeNathees commented Jun 26, 2021

Yes, I want the behavior to match Ruby.

I have a slightly different way, that instead of adding a boolean is_exclusive or is_inclusive to the range object, we always keep our range objects as exclusive internally.

  • This will prevent us an extra check for each time we're iterating (ex: if (range->is_inclusive) foo();) for performance.
  • We don't have to check if the range is inclusive or exclusive at different places in our code. This may cause a bug if we missed somewhere.

Instead, we split the opcode OP_RANGE by OP_RANGE_IN, OP_RANGE_EX, (see pk_opcodes.h). When we create a new range we set the end value depends on the opcode like this. (totally untested code)

pk_compiler.c - static void exprBinaryOp(Compiler* compiler)

-    case TK_DOTDOT:     emitOpcode(compiler, OP_RANGE);      break;
     case TK_DOTDOT:     emitOpcode(compiler, OP_RANGE_IN);   break;
     case TK_DOTDOTDOT:  emitOpcode(compiler, OP_RANGE_EX);   break;

pk_vm.c

-  OPCODE(RANGE):
   OPCODE(RANGE_IN):
   OPCODE(RANGE_EX):
    {
      Var to = PEEK(-1);   // Don't pop yet, we need the reference for gc.
      Var from = PEEK(-2); // Don't pop yet, we need the reference for gc.
      if (!IS_NUM(from) || !IS_NUM(to)) {
        RUNTIME_ERROR(newString(vm, "Range arguments must be number."));
      }
      DROP(); // to
      DROP(); // from
-     PUSH(VAR_OBJ(newRange(vm, AS_NUM(from), AS_NUM(to))));
      double from = AS_NUM(from), to = AS_NUM(to);
      if (instruction == OP_RANGE_IN) to  = 1;
      PUSH(VAR_OBJ(newRange(vm, from, to)));
      DISPATCH();
    }

@tsujp If you have any suggestions let me know and If you need any help feel free to ping me.

@tsujp
Copy link
Contributor

tsujp commented Jun 27, 2021

@ThakeeNathees

I've got the implementation working but these macros (far above my current level) are too much for me. See the PR as I'm not sure whether to discuss here or there. I'll follow wherever you reply.

PR: #142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requesting for new featuers TODO It hasn't implemented yet.
Projects
None yet
Development

No branches or pull requests

3 participants