Adding support for for loops and simple ranges #216

TobiasWrigstad · 2015-09-11T15:45:27Z

NOTE: there is some code in the code generator that should be
refactored. I invite anyone to fix this. I will not due to
multiple constraints.

Documentation for for loops in updated docs.
Test added to the basic tests.

The for loop explained through example:

for i <- [1..5] print("{} ", i)

prints "1 2 3 4 5". If my_array = [1,2,3,4,5],

for i <- my_array print("{} ", i)

prints "1 2 3 4 5". The following all print "1 3 5":

for i <- my_array by 2 print("{} ", i)
for i <- [1..5] by 2 print("{} ", i)
for i <- [1..5 by 2] print("{} ", i)

The trailing by on the for loop is useful for changing
the stride length when you are iterating over an array
or a range that weren't given as a literal. They do
combine, so 'the crazy' statement:

for i <- [1..5 by 2] by 2 print("{} ", i)

prints "1 5", although it isn't very pretty.

for i <- [1..5] print("{} ", i) prints "1 2 3 4 5". If my_array = [1,2,3,4,5], for i <- my_array print("{} ", i) prints "1 2 3 4 5". The following all print "1 3 5": for i <- my_array by 2 print("{} ", i) for i <- [1..5] by 2 print("{} ", i) for i <- [1..5 by 2] print("{} ", i) The trailing by on the foreloop is useful for changing the stride length when you are iterating over an array or a range that weren't given as a literal. They do combine, so 'the crazy' statement: for i <- [1..5 by 2] by 2 print("{} ", i) prints "1 5". Added a test for for loops

TobiasWrigstad · 2015-09-11T15:52:59Z

Sorry for such a huge PR… I guess I could have done ranges separately, but the for loop one would still have ben huge.

EliasC · 2015-09-15T11:13:36Z

When trying to build this, I get the error

make[3]: *** No rule to make target `../range/range.c', needed by `obj/debug/Debug/range/range.o'.  Stop.

It seems you forgot to add the range library.

EliasC · 2015-09-15T11:18:52Z

src/back/CCode/PrettyCCode.hs

@@ -104,6 +104,7 @@ pp' (If c t e) = text "if" <+> parens  (pp' c) $+$
                   braced_block (pp' t) $+$
                 text "else" $+$
                   braced_block (pp' e)
+pp' (RangeLiteral start stop step) = text "[" <+> pp' start <+> pp' stop <+> pp' step <+> text "]"


This seems weird. Shouldn't this be printing C-code? It looks like [0..10] by 2 would translate to the "C-code" [ 0 10 2 ].

Upon closer inspection it seems like this node of the CCode-AST is never used anywhere. I guess it could be removed from CCode.Main.

I will leave this fix to someone who know what s/he is doing.

EliasC · 2015-09-15T11:27:43Z

@kikofernandez: Do you know how Jenkins manages to pass all the tests when there are files missing (range.c and range.h aren't in the commit)? Isn't it a make clean; make test sort of thing?

EliasC · 2015-09-15T11:36:13Z

src/ir/AST/Desugarer.hs

+--                                      PLUS
+--                                      (VarAccess emeta name)
+--                                      (VarAccess emeta (Name "__step__"))))]))
+


These comments should be removed. Also, should we get rid of the repeat node while we're at it? I think the for-loop does the same thing in a nicer way (and with a less confusing syntax).

About 100% of the programs I have written uses repeat. We can't deprecate it now. Maybe later.

100% of what? And where are these programs deployed? Seems like it would be a simple regex-replace. The longer we wait, the more programs will be written that uses repeat, making deprecation harder.

Maybe this could be a catalyst for adding warnings to the compiler. "Warning: repeat will soon be deprecated" :)

100% of the Encore programs I have written.

Some of these programs are used for writing papers at CWI, etc. with deadlines soon. Would be bad if they stopped working. On top of that, there are bachelor students writing programs and master students writing programs — possibly as we "speak". At least 10 people. If we remove features from the compiler, their programs must change. The compiler is shaky enough as it is … they don't need us changing things under foot.

I think deprecation warnings are a good idea. We can even put a date for when they will be gone. And "use this instead".

EliasC · 2015-09-16T08:09:13Z

I just pushed some minor things. Here are a few additional things to discuss:

for-loops always have the type void, while while-loops have the type of their bodies. We should have them do the same thing
There is a foreach-loop that seems to do what a for-loop does but only for arrays (and using desugaring). It seems for subsumes foreach, so I suggest removing foreach, at least soon. However, the syntax of foreach uses in instead of <-, which I like more. I propose we change the syntax of for to for i in [1..10] by 2.
The type Range is not accessible in the surface language (Range is interpreted as a class or trait type).
Right now using non-positive step lengths in ranges and for-loops hangs the resulting programs. For negative step lengths I could imagine going through the collection in reverse:
```
for i in [0..10] by -2
  print i
----
10
8
6
4
2
0
```
For zero length steps I'm not sure, but throwing the program into an infinite loop seems like the wrong thing to do...

I could fix 1. and 3. right away if there are no objections, but 2. and 4. need some more discussion! After these issues have been resolved I think this PR is ready to be merged.

TobiasWrigstad · 2015-09-16T08:20:12Z

Thanks for the comments!

for-loops always have the type void, while while-loops have the type of their bodies. We should have them do the same thing

Agreed. What does a loop return? The value of the last expression?
I have no strong opinion on what the right return type should be.

Point of order: what is the process of deciding what's right?

There is a foreach-loop that ...

I agree with retiring this. I didn't know about it before. I believe
Kiko is the person behind it. Make sure it is not used in Par T
types.

However, the syntax of foreach ...

For now, lets roll with something similar to the repeat loop and
we can talk syntax in a more systematic way later.

The type Range is not accessible in the surface language (Range is interpreted as a class or trait type).

This is correct. Once we have a standard library in place, we can
address that. For now, I thought having working for loops was more
urgent.

Right now using non-positive step lengths in ranges and for-loops hangs the resulting programs.

That is not good.

For negative step lengths I could imagine going through the collection in reverse:

That is a good improvement. Will you make this change or should I?

For zero length steps I'm not sure, but throwing the program into an infinite loop seems like the wrong thing to do...

I suggest we throw an assert error.

I could fix 1. and 3. right away if there are no objections

Please do!

and 4. need some more discussion! After these issues have been resolved I think this PR is ready to be merged.

I think we know what to do about 4. (Union of our comments.)

Also, I propose we deal with 2 in a different venue. It is even simple to
support parallel syntax (in, <-) if backwards compatibility is a problem.

EliasC · 2015-09-16T08:30:51Z

Agreed. What does a loop return? The value of the last expression?

That's what while-loops do, so let's go with that for now.

Point of order: what is the process of deciding what's right?

Discussing it here on GitHub and giving everyone interested time to join the discussion is how we've done it so far. If there are (bi)weekly Encore-meetings we could have talk about it there!

For now, lets roll with something similar to the repeat loop and we can talk syntax in a more systematic way later.

Well, I'm rooting for getting rid of the repeat statement altogether as you know... :)

I can fix 1, 3 and 4 right away. That gives me a chance to look closer at the code as well.

TobiasWrigstad · 2015-09-16T08:50:35Z

I don't like making "big" "design" decisions from discussions on GitHub. They aren't easy to find later. There needs to be a better process.

Please fix 1, 3 and 4!

Maybe we need both experimental and deprecated annotations?

This is done via a runtime check that terminates the program if the step length is 0 in `for`-loops or ranges.

The value of the last iteration of the loop is the value of the whole loop-expression.

EliasC · 2015-09-16T09:50:19Z

1 and half of 4 done!

In `for i <- rng [1..10]`, the parser saw `rng[1..10]` as a malformed array access. Adding a `try` for array accesses solved it.

Here is an example program that works: ``` def foo(rng : Range) : Range for i <- rng [1..i] class Main def main() : void for i <- foo([1..10]) by 2 print i ```

This commit also unifies the two cases of iterating over an array and a range. However, it also disables the optimization that avoids creating a range when possible. Will come back for that later.

kikofernandez · 2015-09-19T15:55:42Z

src/parser/Parser/Parser.hs

@@ -580,3 +586,20 @@ expr  =  unit
      real = do pos <- getPosition
                r <- float
                return $ RealLiteral (meta pos) r
+      for = do pos <- getPosition -- for i <- 1..10 by 2 expr


is the comment valid? I thought it was for i in [1..10] by 2 expr

You are right, there should be angle brackets!

(and really, I think this comment could go)

albertnetymk · 2015-09-19T22:32:48Z

Why is it need to have range type in the runtime? The code in range.c looks quite similar to the ones in array.c.

kikofernandez · 2015-09-22T08:55:32Z

it's in status Ready for review but I think this is incorrect. There a some minimal changes to do. I am going to move it back to In Progress.

TobiasWrigstad · 2015-09-22T09:29:36Z

Thanks Kiko. What are the minimal changes?

@albertnetymk:
Ranges can be passed around at run-time, they are proper objects. You can pass a range to another function etc. Ranges don't start at 0 as arrays do, they can have a stride other than 1 (which all arrays have) and they simply contain information about start, stop and stride -- not the entire content of the array (as arrays do).

kikofernandez · 2015-09-22T09:50:16Z

I think it's enough with the comments where @EliasC agreed. I can do them if no one has time.
Not sure I agree with @albertnetymk comment. That can be a refactoring if necessary.

EliasC · 2015-09-22T12:00:42Z

@kikofernandez You are right! The label should be in progress (I will open a Waffle-tab so that I'm reminded about these things). I can fix the things you suggested right away.

albertnetymk · 2015-09-22T12:56:07Z

You can pass a range to another function etc.

@TobiasWrigstad Could you extend the test file to cover this case so that I know how it looks like?

EliasC · 2015-09-22T13:14:32Z

Fixed minor suggestions by @kikofernandez. Also extended the test-file with a function (@albertnetymk):

def showRange(r : Range) : void
  for i in r
    print i
...
    let
      ...
      r = [1..10]
    in {
      ...
      showRange(r);
      ...

albertnetymk · 2015-09-22T13:17:46Z

@EliasC Clear. Thanks.

kikofernandez · 2015-09-22T14:24:20Z

sorry, was closing using waffle and it closes the PR and couldn't merge

Adding support for for loops and simple ranges

TobiasWrigstad · 2015-09-22T14:45:20Z

Is this closed properly now?

kikofernandez · 2015-09-22T14:46:59Z

yes, it's been merged and closed.

TobiasWrigstad · 2015-09-22T14:47:42Z

Nice, thanks!

EliasC reviewed Sep 15, 2015
View reviewed changes

TobiasWrigstad added 2 commits September 15, 2015 14:57

Added missing ranges

4ef9b70

Fixing the comments from Elias

3d5fe54

TobiasWrigstad added the ready label Sep 15, 2015

TobiasWrigstad assigned EliasC Sep 15, 2015

EliasC added 3 commits September 16, 2015 09:47

Minor cleanup

08d933d

Added for and by as keywords in the emacs mode

470b94f

Refactored typechecking of for-loops

80ee82a

EliasC added 2 commits September 16, 2015 11:30

Step length cannot be 0

54c7676

This is done via a runtime check that terminates the program if the step length is 0 in `for`-loops or ranges.

for-loop now has the type of its body

6cf9138

The value of the last iteration of the loop is the value of the whole loop-expression.

EliasC added 5 commits September 16, 2015 13:56

Fixed bug where one couldn't iterate over an array

c1d967a

Fixed parsing issue with ranges

f6878f4

In `for i <- rng [1..10]`, the parser saw `rng[1..10]` as a malformed array access. Adding a `try` for array accesses solved it.

Make the Range type usable by the programmer

0374a5f

Here is an example program that works: ``` def foo(rng : Range) : Range for i <- rng [1..i] class Main def main() : void for i <- foo([1..10]) by 2 print i ```

Changed syntax of for-loops to use in

ebe0f21

Negative step length now traverses backwards

da33536

This commit also unifies the two cases of iterating over an array and a range. However, it also disables the optimization that avoids creating a range when possible. Will come back for that later.

kikofernandez reviewed Sep 19, 2015
View reviewed changes

kikofernandez added in progress and removed ready for review labels Sep 22, 2015

Minor cleanup

0c0a623

EliasC added ready for review and removed in progress labels Sep 22, 2015

kikofernandez added in review and removed ready for review labels Sep 22, 2015

kikofernandez closed this Sep 22, 2015

kikofernandez removed the in review label Sep 22, 2015

kikofernandez reopened this Sep 22, 2015

kikofernandez added the ready for review label Sep 22, 2015

kikofernandez pushed a commit that referenced this pull request Sep 22, 2015

Merge pull request #216 from TobiasWrigstad/for-loops-and-ranges

2164867

Adding support for for loops and simple ranges

kikofernandez merged commit 2164867 into parapluu:master Sep 22, 2015

kikofernandez removed the ready for review label Sep 22, 2015

kikofernandez deleted the for-loops-and-ranges branch September 22, 2015 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for for loops and simple ranges #216

Adding support for for loops and simple ranges #216

TobiasWrigstad commented Sep 11, 2015

TobiasWrigstad commented Sep 11, 2015

EliasC commented Sep 15, 2015

EliasC Sep 15, 2015

EliasC Sep 15, 2015

TobiasWrigstad Sep 15, 2015

EliasC commented Sep 15, 2015

EliasC Sep 15, 2015

TobiasWrigstad Sep 15, 2015

EliasC Sep 15, 2015

TobiasWrigstad Sep 15, 2015

EliasC commented Sep 16, 2015

TobiasWrigstad commented Sep 16, 2015

EliasC commented Sep 16, 2015

TobiasWrigstad commented Sep 16, 2015

EliasC commented Sep 16, 2015

kikofernandez Sep 19, 2015

EliasC Sep 20, 2015

EliasC Sep 20, 2015

albertnetymk commented Sep 19, 2015

kikofernandez commented Sep 22, 2015

TobiasWrigstad commented Sep 22, 2015

kikofernandez commented Sep 22, 2015

EliasC commented Sep 22, 2015

albertnetymk commented Sep 22, 2015

EliasC commented Sep 22, 2015

albertnetymk commented Sep 22, 2015

kikofernandez commented Sep 22, 2015

TobiasWrigstad commented Sep 22, 2015

kikofernandez commented Sep 22, 2015

TobiasWrigstad commented Sep 22, 2015

Adding support for for loops and simple ranges #216

Adding support for for loops and simple ranges #216

Conversation

TobiasWrigstad commented Sep 11, 2015

TobiasWrigstad commented Sep 11, 2015

EliasC commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliasC commented Sep 15, 2015

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

EliasC commented Sep 16, 2015

TobiasWrigstad commented Sep 16, 2015

EliasC commented Sep 16, 2015

TobiasWrigstad commented Sep 16, 2015

EliasC commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertnetymk commented Sep 19, 2015

kikofernandez commented Sep 22, 2015

TobiasWrigstad commented Sep 22, 2015

kikofernandez commented Sep 22, 2015

EliasC commented Sep 22, 2015

albertnetymk commented Sep 22, 2015

EliasC commented Sep 22, 2015

albertnetymk commented Sep 22, 2015

kikofernandez commented Sep 22, 2015

TobiasWrigstad commented Sep 22, 2015

kikofernandez commented Sep 22, 2015

TobiasWrigstad commented Sep 22, 2015