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

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: lib/Sema/SemaType.cpp:1569
@@ +1568,3 @@
+        // Mark them as invalid.
+        attr.setInvalid();
+      }
----------------
manmanren wrote:
> rjmccall wrote:
> > It's not generally a good idea to set things as invalid if you're just emitting a warning.  It might be different for parsed AttributeList objects, but... I'm not sure about that.
> Here we mark the AttributeList as invalid so when we call processTypeAttrs later, we will ignore these attributes, instead of creating AttributedType based on DependentTy for omitted block return type.
I'm just worried that there might be code which suppresses *error* diagnostics (or semantic analysis) when it sees an invalid attribute.  Like I said, though, that might not be a problem for AttributeList.

================
Comment at: test/SemaOpenCL/invalid-block.cl:9
@@ -8,3 +8,3 @@
   int (^const bl2)(); // expected-error{{invalid block variable declaration - must be initialized}}
-  int (^const bl3)() = ^const(){
+  int (^const bl3)() = ^(){ // expected-error{{incompatible block pointer types initializing 'int (^const)()' with an expression of type 'void (^)(void)'}}
   };
----------------
Anastasia wrote:
> Anastasia wrote:
> > I think this test had a bug initially (multiple actually!). It was meant to initialize with int(^)() expression. Could you please add an int return so that this line has no error. 
> I am not getting why this error didn't appear before your change. How does your change trigger this error now, as you seem to be only changing the attributes and not the deduced block type...
Probably because of the unfortunate choice in block semantic analysis to use DependentTy as the marker for an unspecified block result type rather than some other placeholder type (like the undeduced-auto placeholder).  This will cause the block to have a dependent type and basically disable a lot of downstream analysis if, as here, the placeholder is never actually replaced.

================
Comment at: test/SemaOpenCL/invalid-block.cl:39
@@ -38,3 +38,3 @@
 void f5(int i) {
-  bl1_t bl1 = ^const(int i) {return 1;};
-  bl1_t bl2 = ^const(int i) {return 2;};
+  bl1_t bl1 = ^(int i) {return 1;};
+  bl1_t bl2 = ^(int i) {return 2;};
----------------
Anastasia wrote:
> So the return type is deduced to be int for this block expression here?
Yes.  Block return types are deduced according to the types of the operands of the return statements within the block.  The deduction rule is somewhat more permissive / complex than the lambda deduction rule.


http://reviews.llvm.org/D18567