[llvm-commits] [PATCH 01/13] Add For Loop Structures

[David A. Greene]

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

> On Jan 31, 2012, at 2:49 PM, David Greene wrote:
>
>     Add some data structures to represent for loops.  These will be
>     referenced during object processing to do any needed iteration and
>     instantiation.
>
> - Don't add trailing whitespace.

Ok.

> - Clean up comments after cut/paste (ParseForeachDeclaration).

Ok.

> - Don't put the loop stack in RecordKeeper. It is a parse time data structure.
>
>     struct ForeachLoop {
>       Record *Rec;          // Placeholder for body contents.

Ok.

> MultiClass has 'Record Rec;', no pointer. Why the difference?

I can change this, I think.

>       // Create a second member to hold the list value.  We know that
>       // CurRec is the foreach loop record so this won't conflict with
>       // anything.
>       ItemName = BinOpInit::get(BinOpInit::STRCONCAT, ItemName,
>                                 StringInit::get("_List"),
>                                 StringRecTy::get())->Fold(CurRec, 0);
>       ForeachListName = ItemName;
>
> Why store the list as a record field? Can't you just store a ListInit
> directly in ForEachLoop?

That's how I started but I found it more convenient this way.  I confess
I don't remember the details.  I'll look at this again.

> - What's DefPrototypes for? I don't see it used anywhere.

Probably an artifact.  I'll remove it.

> - The syntax in the documentation doesn't match the implementation and tests.

I'll double-check.

> - Please avoid putting spurious records in RecordKeeper. It shouldn't
> be necessary to store unnamed loop and iteration records there.

Ok.

> When resubmitting, please squash everything into one patch.

Are you sure?  Incremental development is the preferred approach.

                           -Dave