[llvm-commits] [PATCH] Revised LandingPadInst Patch

[Chris Lattner]

On Aug 11, 2011, at 11:59 AM, Bill Wendling wrote:
>>> Ah! I think I understand. So it's not so much that the arrays would be splatted out into the OperandList, but that they would be referenced from there (or something).
>>> 
>>> BTW, care must be taken. An empty filter has a specific meaning in this scheme: it's equivalent to a "throw()" in C++.
>>> 
>> 
>> That worked! Here's the revised patch. It now has no more (obvious) grossness in the LandingPadInst instruction. I modified the LangRef.html doc to show this change.

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.

Overall, the patch looks great, please commit it with these changes.  Thanks Bill!

-Chris