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

[Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL266648: Block: Fix a crash when we have type attributes or 
qualifiers with omitted (authored by mren).

Changed prior to commit:
  http://reviews.llvm.org/D18567?vs=52398=54090#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18567

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/SemaObjC/block-omitted-return-type.m
  cfe/trunk/test/SemaOpenCL/invalid-block.cl

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8437,4 +8437,12 @@
   "parameterized class %0 already conforms to the protocols listed; did you "
   "forget a '*'?">, InGroup;
 
+def warn_block_literal_attributes_on_omitted_return_type : Warning<
+  "attribute %0 ignored, because it cannot be applied to omitted return type">,
+  InGroup;
+
+def warn_block_literal_qualifiers_on_omitted_return_type : Warning<
+  "'%0' qualifier on omitted return type %1 has no effect">,
+  InGroup;
+
 } // end of sema component.
Index: cfe/trunk/test/SemaOpenCL/invalid-block.cl
===
--- cfe/trunk/test/SemaOpenCL/invalid-block.cl
+++ cfe/trunk/test/SemaOpenCL/invalid-block.cl
@@ -4,16 +4,15 @@
 
 // All blocks declarations must be const qualified and initialized.
 void f1() {
-  int (^bl1)() = ^() {}; // expected-error{{invalid block variable declaration - must be const qualified}}
+  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;};
 }
 
 // A block with extern storage class is not allowed.
-extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+extern int (^const bl)() = ^(){return 1;}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
 void f2() {
-  extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+  extern int (^const bl)() = ^(){return 1;}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
 }
 
 // A block cannot be the return value of a function.
@@ -29,22 +28,22 @@
 }
 
 // A block with variadic argument is not allowed.
-int (^const bl)(int, ...) = ^const int(int I, ...) { // expected-error {{invalid block prototype, variadic arguments are not allowed in OpenCL}}
+int (^const bl)(int, ...) = ^int(int I, ...) { // expected-error {{invalid block prototype, variadic arguments are not allowed in OpenCL}}
   return 0;
 };
 
 // A block can't be used to declare an array
 typedef int (^const bl1_t)(int);
 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;};
   bl1_t arr[] = {bl1, bl2}; // expected-error {{array of 'bl1_t' (aka 'int (^const)(int)') type is invalid in OpenCL}}
   int tmp = i ? bl1(i)  // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
   : bl2(i); // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
 }
 
 void f6(bl1_t * bl_ptr) {
-  bl1_t bl = ^const(int i) {return 1;};
+  bl1_t bl = ^(int i) {return 1;};
   bl1_t *p =  // expected-error {{invalid argument type 'bl1_t' (aka 'int (^const)(int)') to unary expression}}
   bl = *bl_ptr;  // expected-error {{dereferencing pointer of type '__generic bl1_t *' (aka 'int (^const __generic *)(int)') is not allowed in OpenCL}}
 }
Index: cfe/trunk/test/SemaObjC/block-omitted-return-type.m
===
--- cfe/trunk/test/SemaObjC/block-omitted-return-type.m
+++ cfe/trunk/test/SemaObjC/block-omitted-return-type.m
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 %s -fblocks -verify -fsyntax-only
+
+@interface NSObject
+@end
+
+@interface Test : NSObject
+- (void)test;
+@end
+
+@implementation Test
+- (void)test
+{
+  void (^simpleBlock)() = ^ _Nonnull { //expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}}
+return;
+  };
+  void (^simpleBlock2)() = ^ _Nonnull void { //expected-error {{nullability specifier '_Nonnull' cannot be applied to non-pointer type 'void'}}
+return;
+  };
+  void (^simpleBlock3)() = ^ _Nonnull (void) {  //expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}}
+return;
+  };
+
+  void 

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

[Anastasia Stulova via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM!


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via cfe-commits]

rjmccall added a comment.

Seems okay to me.  Normally we wouldn't want to remove an attribute from the 
DeclSpec because it can apply to multiple declarators, but that's not possible 
with a block-literal declarator, so it's fine.  Richard should sign off, though.


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manman Ren via cfe-commits]

manmanren added a comment.

Ping :]


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manman Ren via cfe-commits]

manmanren updated this revision to Diff 52398.
manmanren added a comment.

Addressing review comments.


http://reviews.llvm.org/D18567

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  test/SemaObjC/block-omitted-return-type.m
  test/SemaOpenCL/invalid-block.cl

Index: test/SemaObjC/block-omitted-return-type.m
===
--- /dev/null
+++ test/SemaObjC/block-omitted-return-type.m
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 %s -fblocks -verify -fsyntax-only
+
+@interface NSObject
+@end
+
+@interface Test : NSObject
+- (void)test;
+@end
+
+@implementation Test
+- (void)test
+{
+  void (^simpleBlock)() = ^ _Nonnull { //expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}}
+return;
+  };
+  void (^simpleBlock2)() = ^ _Nonnull void { //expected-error {{nullability specifier '_Nonnull' cannot be applied to non-pointer type 'void'}}
+return;
+  };
+  void (^simpleBlock3)() = ^ _Nonnull (void) {  //expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}}
+return;
+  };
+
+  void (^simpleBlock4)() = ^ const { //expected-warning {{'const' qualifier on omitted return type '' has no effect}}
+return;
+  };
+  void (^simpleBlock5)() = ^ const void { //expected-error {{incompatible block pointer types initializing 'void (^)()' with an expression of type 'const void (^)(void)'}}
+return;
+  };
+  void (^simpleBlock6)() = ^ const (void) { //expected-warning {{'const' qualifier on omitted return type '' has no effect}}
+return;
+  };
+  void (^simpleBlock7)() = ^ _Nonnull __attribute__((align_value(128))) _Nullable const (void) { // expected-warning {{attribute '_Nullable' ignored, because it cannot be applied to omitted return type}} \
+// expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}} \
+// expected-warning {{'const' qualifier on omitted return type '' has no effect}} \
+// expected-warning {{'align_value' attribute only applies to variables and typedefs}}
+return;
+  };
+  void (^simpleBlock9)() = ^ __attribute__ ((align_value(128))) _Nonnull const (void) { // expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}} \
+// expected-warning {{'const' qualifier on omitted return type '' has no effect}} \
+// expected-warning {{'align_value' attribute only applies to variables and typedefs}}
+return;
+  };
+}
+@end
Index: test/SemaOpenCL/invalid-block.cl
===
--- test/SemaOpenCL/invalid-block.cl
+++ test/SemaOpenCL/invalid-block.cl
@@ -4,16 +4,15 @@
 
 // All blocks declarations must be const qualified and initialized.
 void f1() {
-  int (^bl1)() = ^() {}; // expected-error{{invalid block variable declaration - must be const qualified}}
+  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;};
 }
 
 // A block with extern storage class is not allowed.
-extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+extern int (^const bl)() = ^(){return 1;}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
 void f2() {
-  extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+  extern int (^const bl)() = ^(){return 1;}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
 }
 
 // A block cannot be the return value of a function.
@@ -29,22 +28,22 @@
 }
 
 // A block with variadic argument is not allowed.
-int (^const bl)(int, ...) = ^const int(int I, ...) { // expected-error {{invalid block prototype, variadic arguments are not allowed in OpenCL}}
+int (^const bl)(int, ...) = ^int(int I, ...) { // expected-error {{invalid block prototype, variadic arguments are not allowed in OpenCL}}
   return 0;
 };
 
 // A block can't be used to declare an array
 typedef int (^const bl1_t)(int);
 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;};
   bl1_t arr[] = {bl1, bl2}; // expected-error {{array of 'bl1_t' (aka 'int (^const)(int)') type is invalid in OpenCL}}
   int tmp = i ? bl1(i)  // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
   : bl2(i); // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
 }
 
 void f6(bl1_t * bl_ptr) {

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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.


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;};
 }

manmanren wrote:
> Yes, John is right. The DependentTy is never replaced if wrapped with an 
> Attributed type or a qualified type.
Ah, I see. And now the error occurs because you are invalidating/removing the 
attributes?


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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  = *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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/Sema/SemaType.cpp:1253-1254
@@ -1252,4 +1253,4 @@
   break;
 } else if (declarator.getContext() == Declarator::LambdaExprContext ||
isOmittedBlockReturnType(declarator)) {
   Result = Context.DependentTy;

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?


Comment at: lib/Sema/SemaType.cpp:1561
@@ +1560,3 @@
+AttributeList *cur = attrs, *prev = nullptr;
+while (cur) {
+  AttributeList  = *cur;

Maybe write this as `for (AttributeList *cur = attrs; cur; cur = 
cur->getNext())` and drop the `cur = cur->getNext()` assignments below.


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();
+}

You can express this more simply as:

  if (prev)
prev->setNext(cur->getNext());
  else
attrs = cur->getNext();
  cur = cur->getNext();



http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manman Ren via cfe-commits]

manmanren added a comment.

Thanks for reviewing!

I will update the patch addressing the comments soon!

Manman



Comment at: lib/Sema/SemaType.cpp:1569
@@ +1568,3 @@
+// Mark them as invalid.
+attr.setInvalid();
+  }

rsmith wrote:
> rjmccall wrote:
> > 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.
> Even if it didn't cause problems today, it's very likely to cause confusion 
> and problems down the line. We should not have two different meanings for 
> "invalid" for different kinds of AST nodes. Manman, can you remove the 
> attribute(s) in question from the attribute list instead?
John and Richard,

I got your point now. I didn't think about removing it from the attribute list 
before.

I looked at the interface for AttributeList, there is no interface for removing 
an item from the list, it has a setNext function.
But I found a helper function called spliceAttrOutOfList, I will try to use it.




http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.


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)'}}
   };

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. 


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:
> 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...


Comment at: test/SemaOpenCL/invalid-block.cl:14
@@ -13,3 +13,3 @@
 // A block with extern storage class is not allowed.
-extern int (^const bl)() = ^const(){}; // expected-error{{invalid block 
variable declaration - using 'extern' storage class is disallowed}}
+extern int (^const bl)() = ^(){}; // expected-error{{invalid block variable 
declaration - using 'extern' storage class is disallowed}}
 void f2() {

The same here, line 7 and 16. Could we return int please.


Comment at: test/SemaOpenCL/invalid-block.cl:32
@@ -31,3 +31,3 @@
 // A block with variadic argument is not allowed.
 int (^const bl)(int, ...) = ^const int(int I, ...) { // expected-error 
{{invalid block prototype, variadic arguments are not allowed in OpenCL}}
   return 0;

could you remove second const please?


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;};

So the return type is deduced to be int for this block expression here?


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manman Ren via cfe-commits]

manmanren added a comment.

Cheers,
Manman



Comment at: lib/Sema/SemaType.cpp:1569
@@ +1568,3 @@
+// Mark them as invalid.
+attr.setInvalid();
+  }

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.


Comment at: lib/Sema/SemaType.cpp:1609
@@ +1608,3 @@
+// Warn if we see type qualifiers for omitted return type on a block 
literal.
+if (TypeQuals && isOmittedBlockReturnType(declarator)) {
+  diagnoseAndRemoveTypeQualifiers(S, DS, TypeQuals, Result,

rjmccall wrote:
> Checking TypeQuals again here is redundant.
You are right, I was following the code below this.


Comment at: lib/Sema/SemaType.cpp:1611
@@ +1610,3 @@
+  diagnoseAndRemoveTypeQualifiers(S, DS, TypeQuals, Result,
+  DeclSpec::TQ_const | DeclSpec::TQ_volatile | DeclSpec::TQ_atomic,
+  diag::warn_block_literal_qualifiers_on_omitted_return_type);

rjmccall wrote:
> You're missing at least TQ_restrict.  But why make this an enumerated list at 
> all?
I was trying to reuse diagnoseAndRemoveTypeQualifiers and didn't notice that it 
does not cover all the qualifiers.
I will rewrite this to not use diagnoseAndRemoveTypeQualifiers.


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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();
+  }

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.


Comment at: lib/Sema/SemaType.cpp:1609
@@ +1608,3 @@
+// Warn if we see type qualifiers for omitted return type on a block 
literal.
+if (TypeQuals && isOmittedBlockReturnType(declarator)) {
+  diagnoseAndRemoveTypeQualifiers(S, DS, TypeQuals, Result,

Checking TypeQuals again here is redundant.


Comment at: lib/Sema/SemaType.cpp:1611
@@ +1610,3 @@
+  diagnoseAndRemoveTypeQualifiers(S, DS, TypeQuals, Result,
+  DeclSpec::TQ_const | DeclSpec::TQ_volatile | DeclSpec::TQ_atomic,
+  diag::warn_block_literal_qualifiers_on_omitted_return_type);

You're missing at least TQ_restrict.  But why make this an enumerated list at 
all?


http://reviews.llvm.org/D18567



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits