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

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

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

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/

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

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

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

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

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

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.

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

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/

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

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

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

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

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

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

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

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 ___

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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?)