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