[llvm-commits] [PATCH] Add Foreach Loop
David A. Greene
dag at cray.com
Fri Feb 10 13:30:24 PST 2012
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
More information about the llvm-commits
mailing list