[PATCH] D12108: [WinEH] Require token linkage in EH pad/ret signatures

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: include/llvm/IR/Instructions.h:37
@@ -36,2 +36,3 @@
 class LLVMContext;
+class CleanupPadInst;
 
----------------
Please sort this relative to the other forward declarations.

================
Comment at: include/llvm/IR/Instructions.h:3713
@@ -3721,3 +3712,3 @@
     assert(NewDest);
-    Op<-1>() = NewDest;
+    Op<-1>() = reinterpret_cast<Value *>(NewDest);
   }
----------------
Why is the `reinterpret_cast` necessary here?

================
Comment at: include/llvm/IR/Instructions.h:4042
@@ -4053,2 +4041,3 @@
                                  Instruction *InsertBefore = nullptr) {
     assert(BB);
+    return new (2) CatchReturnInst(CatchPad, BB, InsertBefore);
----------------
We can also assert that `CatchPad` isn't null.

================
Comment at: lib/AsmParser/LLParser.cpp:2237-2253
@@ -2236,2 +2236,19 @@
   if (Val) {
+    if (OC)
+      switch (OC) {
+      case OperatorConstraint::catchpad:
+        if (!isa<CatchPadInst>(Val)) {
+          P.Error(Loc, "'%" + Name + "' is not a catchpad");
+          return nullptr;
+        }
+        break;
+      case OperatorConstraint::cleanuppad:
+        if (!isa<CleanupPadInst>(Val)) {
+          P.Error(Loc, "'%" + Name + "' is not a cleanuppad");
+          return nullptr;
+        }
+        break;
+      default:
+        llvm_unreachable("unexpected constraint");
+      }
     if (Val->getType() == Ty) return Val;
----------------
Could we add a case for `none`? I think it'd let us reduce the indentation here.

================
Comment at: lib/AsmParser/LLParser.h:114-116
@@ +113,5 @@
+    enum OperatorConstraint {
+      none = 0,  // No constraint
+      catchpad,  // Must be CatchPadInst
+      cleanuppad // Must be CleanupPadInst
+    };
----------------
Enumerators should start with a capital letter per the coding standards: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:46-50
@@ +45,7 @@
+/// only reference a certain operator).
+enum OperatorConstraint {
+  none = 0,  // No constraint
+  catchpad,  // Must be CatchPadInst
+  cleanuppad // Must be CleanupPadInst
+};
+
----------------
Likewise with this these enumerators.


http://reviews.llvm.org/D12108