Implement correct for loop behavior by CohenArthur · Pull Request #316 · jinko-core/jinko

CohenArthur · 2021-10-21T13:00:27Z

Closes #132
Closes #322

CohenArthur · 2021-10-24T13:43:06Z

In order to advance the iterator properly, we need variable mutability. We can return a new iterator for now but that's it

CohenArthur · 2021-10-24T14:50:10Z

To progress further, we need comparison operators in order for the Range_int type to act correctly

codecov-commenter · 2021-10-25T08:10:23Z

Codecov Report

Merging #316 (cf5a706) into master (8d8b772) will increase coverage by 0.72%.
The diff coverage is 86.15%.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   92.04%   92.76%   +0.72%     
==========================================
  Files          41       41              
  Lines        4171     4230      +59     
==========================================
+ Hits         3839     3924      +85     
+ Misses        332      306      -26

Impacted Files	Coverage Δ
src/error/mod.rs	`78.08% <ø> (ø)`
src/instruction/loop_block.rs	`85.34% <85.96%> (+32.96%)`	⬆️
src/builtins.rs	`94.68% <87.50%> (-0.78%)`	⬇️
src/parser/constructs.rs	`100.00% <0.00%> (+0.12%)`	⬆️
src/instruction/operator.rs	`90.90% <0.00%> (+3.03%)`	⬆️
src/value/jk_constant.rs	`91.30% <0.00%> (+3.62%)`	⬆️
src/instruction/var.rs	`85.36% <0.00%> (+8.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d8b772...cf5a706. Read the comment docs.

Add println and println_err instead of display

Skallwar

I wonder if this could be done via a jinko! macro combined with a format!. The code is ugly (and that's not your fault) but maybe a jinko! macro will be more elegant and (maybe) more performant?

There is almost no test for this feature, we should add some

Great works 🚀 🔥

Skallwar · 2021-10-25T08:14:07Z

+            __builtin_string_display("to display", false);
+            __builtin_string_display_err("to display on err", true);


Not a big fan of this. If the user wants a new line, he just needs to add a '\n' on his string

Well the issue is that for now these strings aren't parsed correctly... So if we were to do print("hey\n"), then the character \ and the character n would both get printed to the stdout. I think that maybe there should be a FIXME or something in the standard library

Ok let's go with a FIXME and an issue

Done in ba698fd and #323

Skallwar · 2021-10-25T08:16:30Z

@@ -77,32 +77,133 @@ impl Instruction for Loop {
                // that that result, as a boolean, returns true. If it does, execute the
                // body. If it does not, break from the for.


I guess this comment needs to be updated

This is the behavior that I'd still want the loop blocks to have, but yeah it's not implemented right now. I'll add a FIXME

Ooops I read that too fast. You're right it needs updating

Skallwar · 2021-10-25T08:18:22Z

+                // We create new variables in the scope, with the required iterator's
+                // name from the user. We can create these variables with special names
+                // since we are only using them from the interpreter. Here, we prefix
+                // them with a plus sign to make sure they do not interact with the user.


This should be documented. The user needs to be aware that he is not allowed to use identifier starting with a '+' sign

I don't know if it explicitly needs to be documented, as if the user tries to do it they will simply encounter a parse error (identifiers can only be comprised of underscores, digits and letters). Here, since we are creating them directly from the interpreter, we're allowed to, but it's an "edge case" and abusing the language since we're not going through the parser

Documenting it to understand why there is a parser error is useful imao

Do you mean in the parser or here? And do you mean by emitting an error or adding a comment? Because the user will never see those variables, ever. Think of them as the __<ident> variables we were talking about in gcc

Skallwar · 2021-10-25T08:30:09Z

+                // let maybe_is_some = |ctx| {
+                //     let instance = maybe_is_some.execute(ctx).unwrap();
+                //     JkBool::from_instance(&instance).0.clone()
+                // };


dead code ?

Skallwar · 2021-10-25T08:33:23Z

+                counter = counter + 1;
+            }
+
+            // FIXME: Add assertion here


This needs to be fixed I guess

I think it depends on #187 sadly. I'll fix the FIXME

Skallwar · 2021-10-25T08:35:42Z

+                // // FIXME: This needs to change
+                // +iterator = next(+iterator);
+                // +maybe_value = current(+iterator);
+                // if !+maybe_value.is_some() { break }


Are we not doing this already? iterator.next() is in this PR

Yes, but ideally we should simply be mutating the +iterator variable instead of creating a new iterator each time. Something like

func next(iter: &mut Iter) { ... }

Co-authored-by: Skallwar <esteban.blanc@epita.fr>

CohenArthur force-pushed the iter-in-for-loops branch 2 times, most recently from 107f18a to ba56c46 Compare October 25, 2021 07:42

CohenArthur marked this pull request as ready for review October 25, 2021 08:08

CohenArthur requested a review from Skallwar October 25, 2021 08:08

CohenArthur requested a review from SanderJSA October 25, 2021 08:11

CohenArthur force-pushed the iter-in-for-loops branch from 29b2610 to cf5a706 Compare October 25, 2021 08:15

CohenArthur added 13 commits October 25, 2021 10:16

string: Rework display builtins

31949c3

Add println and println_err instead of display

loop_block: Add proper implementation for for loop

f3db4da

stdlib/iter: Add iter.value()

a7de802

error: Display more information in fmt::Display

513070e

stdlib/range: Fix range.current()

4a40704

builtins: Fix tests

b064791

loops: Add functional tests

404193c

stdlib/range: Only iter up to end - 1 included

dbe55bc

loop_block: Remove test for now

42a65d1

loop_block: Add unit test for for loop

cf5a706

builtins: Fix tests to improve coverage

56e6c11

loop_block: Cleanup debug code

92f21be

for_loop: Add more functional tests

e6095aa

Skallwar requested changes Oct 25, 2021

View reviewed changes

loop_block: Add assertion in valid_for_block test

d2e0e25

Co-authored-by: Skallwar <esteban.blanc@epita.fr>

CohenArthur force-pushed the iter-in-for-loops branch from 08cbdc4 to 663d019 Compare October 25, 2021 09:36

CohenArthur and others added 4 commits October 25, 2021 11:37

stdlib/string: Add FIXME for once newlines are handled correctly

ba698fd

Co-authored-by: Skallwar <esteban.blanc@epita.fr>

loop_block: Remove fixed comment

3becc43

Co-authored-by: Skallwar <esteban.blanc@epita.fr>

loop_block: Add more precisions to FIXME

9b19269

Co-authored-by: Skallwar <esteban.blanc@epita.fr>

stdlib/range: Add FIXME for bad code

663d019

Co-authored-by: Skallwar <esteban.blanc@epita.fr>

Skallwar approved these changes Oct 25, 2021

View reviewed changes

CohenArthur merged commit 1f590f8 into master Oct 25, 2021

CohenArthur deleted the iter-in-for-loops branch October 25, 2021 09:54

		__builtin_string_display("to display", false);
		__builtin_string_display_err("to display on err", true);

		@@ -77,32 +77,133 @@ impl Instruction for Loop {
		// that that result, as a boolean, returns true. If it does, execute the
		// body. If it does not, break from the for.

Conversation

CohenArthur commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CohenArthur commented Oct 24, 2021

Uh oh!

CohenArthur commented Oct 24, 2021

Uh oh!

codecov-commenter commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Skallwar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CohenArthur commented Oct 21, 2021 •

edited

Loading

codecov-commenter commented Oct 25, 2021 •

edited

Loading