[llvm-commits] [PATCH] Add Foreach Loop

[David A. Greene]

Jakob Stoklund Olesen <stoklund at 2pi.dk> writes:

> On Feb 9, 2012, at 12:21 PM, David Greene <dag at cray.com> wrote:
>
>> Here is an updated version of the foreach patch series, squashed
>> as requested.  I think I have addressed all of the review feedback.
>> Please review.
>
> The syntax in the documentation is still wrong.

What's wrong about it?

Ack!  Missed the type.  Wow, I just completely read right over that.

>> +  // Foreach management.
>> +
>> +  /// pushLoop - Add a loop to the existing loop context.
>> +  ///
>> +  void pushLoop(const ForeachLoop &loop) {
>> +    Loops.push_back(loop);
>> +  }
>
> This all seems excessive for a private variable. Just use Loops directly?

Ok.

> Please add a test that has sibling nested loops and refs at different levels.
>
> foreach {
>   def
>   foreach {
>     def
>   }
>   def
>   foreach {
>     foreach { … }
>     def
>   }
> }
>
> etc.

Ok.

> Regarding the syntax, I think you should make it parallel with the let keyword:
>
>   foreach string x = […] in { … }

That's reasonable.

> You should also make the braces optional when wrapping a single loop
> or def, just like let does.

I'll have to see how let does that.  I'm not sure I can know early
enough.

> Is it strictly necessary with a type? Let doesn't seem to need it.

I'll check that too.  I'm not certain yet.

                           -Dave