[llvm-commits] [llvm] r167115 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/switch_to_lookup_table.ll

[Hans Wennborg]

Thanks very much for the comments! I've addressed them in r167121.

 - Hans

On Wed, Oct 31, 2012 at 2:40 PM, Duncan Sands <baldrick at free.fr> wrote:
> Hi Hans,
>
>
>> +static Constant* ConstantFold(Instruction *I,
>> +                         const SmallDenseMap<Value*, Constant*>&
>> ConstantPool) {
>> +  if (BinaryOperator *BO = dyn_cast<BinaryOperator>(I)) {
>> +    Constant *A = dyn_cast<Constant>(BO->getOperand(0));
>> +    if (!A) A = ConstantPool.lookup(BO->getOperand(0));
>> +    if (!A) return NULL;
>
>
> LLVM style is to use 0 rather than NULL.

Fixed.

> Also, this "try a dyn_cast, if not
> lookup in the constant pool" logic could (and, I reckon, should) be factored
> out into a helper function rather than repeated many times below.

I've factored this out into LookupConstant.

> Your "if" statements on one line aren't very style conformant, but this one
> I
> particularly don't like because of the control-flow (return 0) in the last
> branch.  How about this:
>   Value *Res = 0;
>    if (A->isAllOnesValue())
>      Res = HelperFunction (Select->getTrueValue());
>    else if (A->isNullValue())
>      Res = HelperFunction (Select->getFalseValue());
>    return Res;
>
> Here HelperFunction is the guy that returns a constant if it could be turned
> into a constant, or null if not.

Right, this is nicer. We don't even need Res actually, we can just
return directly.