For loop for arrays #1753

pmqtt · 2022-07-26T21:29:20Z

Hello folks,
I have the for-loop statement implements... Test Cases are in testdata/for/

Comments and suggestions for improvement are very welcome, because my implementation manipulate the AST to transform the for-loop in the aquivalent while form.

I hope it is useful und best regrads!

explorer/ast/statement.h

geoffromer · 2022-07-28T21:03:55Z

explorer/ast/statement.h

+        body_(body) {
+    Nonnull<Arena*> arena = new Arena();
+    auto index_name =
+        arena->New<IdentifierExpression>(source_loc, "$yy_forloop");


We generally try to have the parsed AST match the syntax that the user supplied, because otherwise things like error messages and trace output can get very confusing, because they're talking about code that the user can't see. We've sometimes talked about implementing some features in terms of AST rewriting, but haven't figured out a design that we're happy with.

Instead of introducing these artificial AST nodes so that they can be fed to the interpreter, my recommendation would be to have logic in the part of the interpreter that implements For that directly does what execution of those nodes would do. For example, instead of synthesizing an AST node that calls __intrinsic_array_sz, have code in the interpreter that directly calls .elements().vec().size() on the array (as a side benefit, I think that means we don't need __intrinsiz_array_sz in the first place). Does that make sense?

Ok! I will remove the code and try to implement as logic in the node.

We may need or want to create additional AST nodes to represent the semantics of the loop -- for example, if we have an interface for converting an arbitrary sequence into something we can iterate over. If you find that's necessary, that would be better done in type_checking.cpp when we type-check the statement, rather than here.

explorer/interpreter/type_checker.cpp

explorer/testdata/for/for-loop.carbon

pmqtt · 2022-07-28T21:44:45Z

Thanks for the review... I should think again, how to implement this...

Co-authored-by: Geoff Romer <[email protected]>

zygoloid · 2022-07-29T19:59:16Z

explorer/ast/expression.cpp

@@ -184,6 +185,9 @@ void Expression::Print(llvm::raw_ostream& out) const {
        case IntrinsicExpression::Intrinsic::Dealloc:
          out << "delete";
          break;
+        case IntrinsicExpression::Intrinsic::ArraySz:
+          out << "array_sz";


This isn't consistent with the spelling you used earlier.

It's removed!

explorer/ast/statement.cpp

zygoloid · 2022-07-29T20:02:46Z

explorer/ast/statement.h

+        body_(body) {
+    Nonnull<Arena*> arena = new Arena();
+    auto index_name =
+        arena->New<IdentifierExpression>(source_loc, "$yy_forloop");


We may need or want to create additional AST nodes to represent the semantics of the loop -- for example, if we have an interface for converting an arbitrary sequence into something we can iterate over. If you find that's necessary, that would be better done in type_checking.cpp when we type-check the statement, rather than here.

# Conflicts: # common/fuzzing/carbon.proto # common/fuzzing/proto_to_carbon.cpp # explorer/ast/expression.cpp # explorer/ast/expression.h # explorer/fuzzing/ast_to_proto.cpp # explorer/interpreter/interpreter.cpp # explorer/interpreter/type_checker.cpp

pmqtt · 2022-07-30T22:48:39Z

Hello folks, I have reimplement the for-loop statement.

Co-authored-by: Richard Smith <[email protected]>

zygoloid · 2022-08-01T23:15:11Z

explorer/interpreter/interpreter.cpp

+                lvalue->address(), source_array->elements()[current_index],
+                stmt.source_loc()));
+          } else {
+            llvm::outs() << "Failure in " << __LINE__ << "\n";


Please use CARBON_CHECK or CARBON_FATAL as appropriate here.

zygoloid · 2022-08-01T23:18:22Z

explorer/interpreter/type_checker.cpp

+      ImplScope inner_impl_scope;
+      inner_impl_scope.AddParent(&impl_scope);


I don't think we should need a separate ImplScope here, since impls can't be nested within the for statement.

Please corrent me, but isn't it so:

Block for( x: i32 in ar) //new Block{ //new Block } Block

Because after the execution of the loop, x shouldn't be found in Block.

Another problem is that TypeCheckPattern expected a non const ImplScope, but impl_scope is const!

zygoloid · 2022-08-01T23:20:20Z

explorer/interpreter/type_checker.cpp

+      CARBON_RETURN_IF_ERROR(TypeCheckPattern(&for_stmt.variable_declaration(),
+                                              std::nullopt, inner_impl_scope,
+                                              ValueCategory::Let));


If we type-check the range first, we can pass in an expected type here, which should allow us to support things like for (x: auto in arr).

thanks, solved!

zygoloid · 2022-08-01T23:21:04Z

explorer/interpreter/type_checker.cpp

+                            &for_stmt.loop_target(),
+                            &for_stmt.variable_declaration().static_type()));


Hm, I would have expected that we want to convert the array element type of the type of the loop target, not the type of the loop target itself, here.

thanks, solved!

# Conflicts: # explorer/syntax/lexer.lpp

explorer/interpreter/interpreter.cpp

explorer/testdata/for/for-loop-sum.carbon

explorer/testdata/for/for-loop-auto.carbon

explorer/testdata/for/for-loop.carbon

explorer/data/prelude.carbon

explorer/interpreter/interpreter.cpp

Co-authored-by: Geoff Romer <[email protected]>

…into for-loop-string

geoffromer

This looks good to me. @zygoloid, do you want to take another look or shall I merge it?

zygoloid

One comment, otherwise looks good.

explorer/interpreter/resolve_names.cpp

Co-authored-by: Richard Smith <[email protected]>

cefour added 13 commits July 26, 2022 08:40

add in operator

a815b76

add for loop in for_each semantic to the grammar

cfd72c4

added For class to statement

9d76d72

early implementation of for

db9a179

for loop draft is complete

ed99411

for loop is implemented

d36a968

add test cases and run pre commit

780b215

add missinng fild name

8b69cb2

add missinng field name

a3fbe85

correct field names

b32e0ea

handle intrinsic and For statement in ast_to_proto

059b316

correct compilation error

8f3cbc1

correct for loop generation in fuzzer

ee3cd54

geoffromer reviewed Jul 28, 2022

View reviewed changes

pmqtt and others added 3 commits July 29, 2022 11:22

Update explorer/testdata/for/for-loop.carbon

c7d059a

Co-authored-by: Geoff Romer <[email protected]>

prettify testdata

0558759

first draft for for-loop

e85a405

zygoloid reviewed Jul 29, 2022

View reviewed changes

cefour added 7 commits July 30, 2022 23:13

for-loop as logic

bfc4bca

refectoring for case in interpreter

2fc36bb

refectoring for case in interpreter finished

7ac7256

add for-loop-sum.carbon

3904dea

run pre-commit

eb16c21

pmqtt requested review from zygoloid and geoffromer July 30, 2022 22:45

Update explorer/ast/statement.cpp

08e0c6b

Co-authored-by: Richard Smith <[email protected]>

run pre-commit

0720724

zygoloid reviewed Aug 1, 2022

View reviewed changes

cefour added 2 commits August 2, 2022 10:44

implement review comments from @zygoloid

c8ce666

run pre-commit

0d13406

pmqtt requested a review from zygoloid August 2, 2022 09:02

cefour added 4 commits August 4, 2022 16:07

Merge remote-tracking branch 'origin/root' into for-loop-string

c075afc

Merge remote-tracking branch 'origin/root' into for-loop-string

e8b426f

# Conflicts: # explorer/syntax/lexer.lpp

add for to unresolve

c7649d1

prelude to bool

3e964ab

geoffromer reviewed Aug 4, 2022

View reviewed changes

cefour added 2 commits August 5, 2022 09:34

implement review comments from @geoffromer and @calvin

e31f97d

run pre-commit

321a065

pmqtt requested a review from geoffromer August 5, 2022 08:03

geoffromer reviewed Aug 8, 2022

View reviewed changes

pmqtt and others added 9 commits August 8, 2022 23:16

Update explorer/interpreter/interpreter.cpp

6553b1c

Co-authored-by: Geoff Romer <[email protected]>

Update explorer/interpreter/interpreter.cpp

3fe7e8d

Co-authored-by: Geoff Romer <[email protected]>

prelude new load

7ba9c73

Merge remote-tracking branch 'origin/root' into for-loop-string

8742de3

run pre-commit

705770c

Merge branch 'for-loop-string' of https://github.com/pmqtt/carbon-lang …

e12cd59

…into for-loop-string

run pre-commit

b2ae799

added changes after review from @geoffromer

6f4b1b3

pre-commit run

a75da43

pmqtt requested a review from geoffromer August 9, 2022 08:17

geoffromer approved these changes Aug 9, 2022

View reviewed changes

zygoloid approved these changes Aug 9, 2022

View reviewed changes

explorer/interpreter/resolve_names.cpp Outdated Show resolved Hide resolved

Update explorer/interpreter/resolve_names.cpp

d0f93b0

Co-authored-by: Richard Smith <[email protected]>

zygoloid merged commit f754373 into carbon-language:trunk Aug 9, 2022

zygoloid mentioned this pull request Aug 25, 2022

Implement for loops #1592

Closed

chandlerc added the explorer Action items related to Carbon explorer code label Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For loop for arrays #1753

For loop for arrays #1753

pmqtt commented Jul 26, 2022

geoffromer Jul 28, 2022

pmqtt Jul 29, 2022

zygoloid Jul 29, 2022

pmqtt commented Jul 28, 2022

zygoloid Jul 29, 2022

pmqtt Jul 30, 2022

zygoloid Jul 29, 2022

pmqtt commented Jul 30, 2022

zygoloid Aug 1, 2022

pmqtt Aug 2, 2022

zygoloid Aug 1, 2022

pmqtt Aug 2, 2022

pmqtt Aug 2, 2022

zygoloid Aug 1, 2022

pmqtt Aug 2, 2022

zygoloid Aug 1, 2022

pmqtt Aug 2, 2022

geoffromer left a comment

zygoloid left a comment

		ImplScope inner_impl_scope;
		inner_impl_scope.AddParent(&impl_scope);

		&for_stmt.loop_target(),
		&for_stmt.variable_declaration().static_type()));

For loop for arrays #1753

For loop for arrays #1753

Conversation

pmqtt commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmqtt commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmqtt commented Jul 30, 2022

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

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

Choose a reason for hiding this comment

geoffromer left a comment

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment