[PATCH] D18567: Block: Fix a crash when we have type attributes or qualifiers with omitted return type.

[Manman Ren via cfe-commits]

manmanren marked 3 inline comments as done.

================
Comment at: lib/Sema/SemaType.cpp:1253-1254
@@ -1252,4 +1253,4 @@
       break;
     } else if (declarator.getContext() == Declarator::LambdaExprContext ||
                isOmittedBlockReturnType(declarator)) {
       Result = Context.DependentTy;
----------------
rsmith wrote:
> Instead of checking for qualifiers below, how about checking here (or, preferably, in `isOmittedBlockReturnType`) whether there were any present, and not treating the block as having an omitted return type if there actually is a return type present?
Makes sense to put related things together .

================
Comment at: lib/Sema/SemaType.cpp:1561
@@ +1560,3 @@
+    AttributeList *cur = attrs, *prev = nullptr;
+    while (cur) {
+      AttributeList &attr = *cur;
----------------
rsmith wrote:
> Maybe write this as `for (AttributeList *cur = attrs; cur; cur = cur->getNext())` and drop the `cur = cur->getNext()` assignments below.
Yes.

================
Comment at: lib/Sema/SemaType.cpp:1575-1583
@@ +1574,11 @@
+      // Remove cur from the list.
+      if (attrs == cur) {
+        attrs = attr.getNext();
+        prev = nullptr;
+        cur = cur->getNext();
+        continue;
+      }
+      prev->setNext(attr.getNext());
+      prev = cur;
+      cur = cur->getNext();
+    }
----------------
rsmith wrote:
> You can express this more simply as:
> 
>   if (prev)
>     prev->setNext(cur->getNext());
>   else
>     attrs = cur->getNext();
>   cur = cur->getNext();
> 
Yes.

================
Comment at: test/SemaOpenCL/invalid-block.cl:8-9
@@ -8,4 +7,4 @@
+  int (^bl1)() = ^() {return 1;}; // expected-error{{invalid block variable declaration - must be const qualified}}
   int (^const bl2)(); // expected-error{{invalid block variable declaration - must be initialized}}
-  int (^const bl3)() = ^const(){
-  };
+  int (^const bl3)() = ^(){return 1;};
 }
----------------
Yes, John is right. The DependentTy is never replaced if wrapped with an Attributed type or a qualified type.


http://reviews.llvm.org/D18567