[LLVMdev] LLVMBuilder vs LLVMFoldingBuilder

[Chris Lattner]

On Apr 10, 2008, at 7:05 AM, Dominic Hamon wrote:

> Dominic Hamon wrote:
>> Duncan Sands wrote:
>>>> Another option that was discussed in #llvm is to nuke LLVMBuilder  
>>>> and rename LLVMFoldingBuilder to LLVMBuilder. If this was the  
>>>> case, I'd argue for a flag in the Builder that could retain the  
>>>> old non-folding functionality for debugging purposes.
>>>>
>>>
>>> this plan sounds good to me.  However it's not clear to me how  
>>> useful a
>>> debug flag would really be.
>>>
>>>
>>
>> This is my first patch so please let me know if there are any  
>> problems or anything I need to do differently.

>>
> And there were. updated patches attached.

Looking good.  One big comment: Please attach patches to email instead  
of including them inline.

Some nits:

> +  Value *CreateMul(Value *LHS, Value *RHS, const char *Name = "") {
> +    if (Constant *LC = dyn_cast<Constant>(LHS))
> +        if (Constant *RC = dyn_cast<Constant>(RHS))
> +            return ConstantExpr::getMul(LC, RC);
> +    return Insert(BinaryOperator::createMul(LHS, RHS, Name));
> +  }

Please consistently indent by 2, not 4.

>
> +
> +    Value *CreateInsertElement(Value *Vec, Value *NewElt, Value *Idx,
> +                                         const char *Name = "") {

Very picky, but 'const char *Name' should line up with 'Value *Vec'.

> Index: gcc/llvm-convert.cpp
> ===================================================================
> --- gcc/llvm-convert.cpp	(revision 49475)
> +++ gcc/llvm-convert.cpp	(working copy)

Please attach llvm-gcc patches as a separate patch file.

Otherwise looks great!

-Chris