[llvm-commits] [PATCH] Revised LandingPadInst Patch

[Bill Wendling]

On Aug 12, 2011, at 11:36 AM, Chris Lattner wrote:

> Sorry for the delay, this is looking much nicer.
> 
> Please add doxygen comments above the create methods:
> 
> +++ include/llvm/Instructions.h	(working copy)
> 
> +  static LandingPadInst *Create(Type *RetTy, Value *PersonalityFn,
> +                                unsigned NumReservedValues,
> +                                const Twine &NameStr = "",
> +                                Instruction *InsertBefore = 0);
> +  static LandingPadInst *Create(Type *RetTy, Value *PersonalityFn,
> +                                unsigned NumReservedValues,
> +                                const Twine &NameStr, BasicBlock *InsertAtEnd);
> 
> In particular, what is "NumReservedValues"?  It isn't clear how that corresponds to clauses. Is this "NumReservedClauses or something else?
> 
> 
> +  /// addClause - Add a clause to the landing pad.
> +  void addClause(ClauseType CT, Value *ClauseVal);
> 
> Since there are only two clause types, it seems much more convenient to have "addFilter" and "addCatch" methods.  Likewise, instead of getClause() have isFilter(...)/isCatch(..) methods.  Providing the Enum APIs in addition to these is fine of course.
> 
> Instead of "getClause" returning the value by-reference, please have it return the Value* and require the client to use isCatch/isFilter.
> 
I made these changes. I actually kept the "getClause" method because both "getCatch" and "getFilter" were identical. Perhaps we should encode the clause type into the OperandList? Maybe something like:

void addCatch(Value *Val) {
  // Grow OperandList if needed.
  OperandList[n  ] = ConstantInt::get(LandingPadInst::Catch);
  OperandList[n+1] = Val;
}

What do you think?

> Overall, the patch looks great, please commit it with these changes.  Thanks Bill!
> 
Thanks Chris! :-)

-bw