[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Thanks everyone!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbc6b139392f6: [clang][parser] Dont prohibit attributes 
on objc @try/@throw (authored by tbaeder).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/CodeGenObjC/attr-nomerge.m


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -triple x86_64-unknown-linux 
-o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(...) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -triple x86_64-unknown-linux -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(...) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 332539.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/CodeGenObjC/attr-nomerge.m


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -triple x86_64-unknown-linux 
-o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(...) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -triple x86_64-unknown-linux -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(...) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D97371#2641800 , @rjmccall wrote:

> Windows exceptions code-generation is quite different; I don't know whether 
> Clang supports ObjC on Windows in general.  It'd be fine if you add a 
> `-triple` argument to this test.

I'd have no issue with that either.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Windows exceptions code-generation is quite different; I don't know whether 
Clang supports ObjC on Windows in general.  It'd be fine if you add a `-triple` 
argument to this test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 332283.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/CodeGenObjC/attr-nomerge.m


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(...) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(...) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+
+  return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Hmm, the new test seems to cause an assertion failure in llvm code generation 
in Windows. Is anything known about that? Is the test case wrong in some way?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 332213.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/CodeGenObjC/attr-nomerge.m


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+@class C;
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(C *c) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+@class C;
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(C *c) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[John McCall via Phabricator via cfe-commits]

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

LGTM, thanks for seeing this through


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 331817.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

Files:
  clang/lib/Parse/ParseStmt.cpp
  clang/test/CodeGenObjC/attr-nomerge.m


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+@class C;
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(C *c) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }


Index: clang/test/CodeGenObjC/attr-nomerge.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/attr-nomerge.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-llvm -fobjc-exceptions -o - %s | FileCheck %s
+
+// Test that the nomerge attribute is applied to function calls
+// in @try, @catch and @finally
+void opaque(void);
+void opaque2(void);
+void opaque3(void);
+
+@class C;
+
+int main(int argc, const char * argv[]) {
+  __attribute__((nomerge)) @try {
+opaque();
+  } @catch(C *c) {
+opaque2();
+  } @finally {
+opaque3();
+  }
+return 0;
+}
+
+// CHECK: call void @opaque() #[[ATTR0:[0-9]+]]
+// CHECK-DAG: call void @opaque2() #[[ATTR0]]
+// CHECK-DAG: call void @opaque3() #[[ATTR0]]
+// CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yes, I think that would be reasonable, thank you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Thank you.

I'm not looking at this test case:

  void opaque(void);
  void opaque2(void);
  void opaque3(void);
  
  @class C;
  
  int main(int argc, const char * argv[]) {
__attribute__((nomerge)) @try {
  opaque();
} @catch(C *c) {
  opaque2();
} @finally {
  opaque3();
}
  return 0;
  }

and compiling with `-S -emit-llvm`

shows:

  %0 = type opaque
  
  ; Function Attrs: noinline nounwind optnone uwtable
  define dso_local i32 @main(i32 %argc, i8** %argv) #0 {
  entry:
; ...
call void @opaque() #2
; ...
  
  cleanup:  ; preds = %entry, %catch
%cleanup.dest.saved = load i32, i32* %cleanup.dest.slot, align 4
call void @opaque3() #2
%finally.shouldthrow = load i1, i1* %finally.for-eh, align 1
br i1 %finally.shouldthrow, label %finally.rethrow, label %finally.cont
  
  catch:; No predecessors!
; ...
call void @opaque2() #2
; ...
  
  attributes #2 = { nomerge }

So all three function calls have the `nomerge` attribute.

I can't find an existing test case checking that `nomerge`, shall I just add 
this one in `clang/test/CodeGenObjC` in this patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D97371#2596643 , @tbaeder wrote:

> They are being applied to the `@try` at least (`-ast-print` prints them) and 
> we do some error checking for missing call expressions in 
> `handleNoMergeAttr()` in `SemaStmtAttr.cpp`. I don't know much about 
> Objective C so I am not sure how to check that the attribute really has any 
> effect in the end.

It's just like `try`/`catch`/`finally` in languages like Java, where you can 
put arbitrary statements inside the nested blocks.  The attribute should affect 
all child statements.  Just find an existing `nomerge` test case that checks 
that IRGen is affected, put that code inside a `@try {  } 
@catch(...) {}`, and verify that the IRGen for that code is still affected.  
Maybe also verify that it works if you put it inside the `@catch` or `@finally` 
block.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

John, do you have any more comments on this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

They are being applied to the `@try` at least (`-ast-print` prints them) and we 
do some error checking for missing call expressions in `handleNoMergeAttr()` in 
`SemaStmtAttr.cpp`. I don't know much about Objective C so I am not sure how to 
check that the attribute really has any effect in the end.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We should never silently accept and ignore an attribute unless (1) that's 
allowable semantics for the attribute or (2) we have to for source 
compatibility.  That test is specifically checking that we allow 
`__attribute__((nomerge))` before `@try` statements.  Are we just dropping the 
attribute, or are we correctly applying it to calls within the statement?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Well we test them on `@try` in `tests/Parser/stmt-attributes.m`, but they are 
not being diagnosed as invalid. Should I instead keep the 
`ProhibitAttributes()` call and change the test to make sure they //are// being 
diagnosed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We don't have any reason to allow attributes on any of the `@` statements, but 
there's no reason to disallow them grammatically, as long as we still diagnose 
them as invalid (which I assume is tested somewhere?).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rjmccall, erik.pilkington.
aaron.ballman added a comment.

Adding some more reviewers who know ObjC better than I do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97371/new/

https://reviews.llvm.org/D97371

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

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, rsmith.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Clang uses GNU-style attributes in objc code in (for example, I guess?) 
`tests/Parser/stmt-attributes.m`:

  __attribute__((nomerge)) @try {
[getTest() foo];
  } @finally {
  }

Once the `ProhibitAttributes()` call in question actually starts handling 
GNU-style attributes, these tests fail.

The call was introduced in c202b2809ac814bcae8553cd772ec4901fdb8441 by Richard 
Smith (I'll add him as a reviewer), but with a `TODO` comment stating that it 
might be incorrect.

I'm having a hard time finding any concrete information on whether GNU-style 
attributes on `@try`/`@catch` statements are allowed in objc, so maybe someone 
with more experience can comment?

Thanks,
Timm


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97371

Files:
  clang/lib/Parse/ParseStmt.cpp


Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }


Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -172,7 +172,6 @@
   switch (Kind) {
   case tok::at: // May be a @try or @throw statement
 {
-  ProhibitAttributes(Attrs); // TODO: is it correct?
   AtLoc = ConsumeToken();  // consume @
   return ParseObjCAtStatement(AtLoc, StmtCtx);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits