[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

In D84005#2215685 , @thakis wrote:

> Looks like this breaks check-clangd on Windows: 
> http://45.33.8.238/win/21943/step_9.txt

Update from an offline conversation: This may actually be rather an issue with 
the test, which was introduced just today: https://reviews.llvm.org/D80525. 
We're looking into reverting that instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like this breaks check-clangd on Windows: 
http://45.33.8.238/win/21943/step_9.txt

Please take a look, and revert while you investigate if the fix takes a while.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa5b8757506b0: Introduce ns_error_domain attribute. (authored 
by MForster, committed by gribozavr).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-print-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -329,6 +329,8 @@
 // empty string but are then recorded as a nullptr.
 OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";
   else if (type == "TypeSourceInfo *")
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -15,3 +15,14 @@
 int fun_asm() asm("test");
 // CHECK: int var_asm asm("test");
 int var_asm asm("test");
+
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 285329.
MForster added a comment.

Rebase before submission


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-print-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -329,6 +329,8 @@
 // empty string but are then recorded as a nullptr.
 OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";
   else if (type == "TypeSourceInfo *")
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -15,3 +15,14 @@
 int fun_asm() asm("test");
 // CHECK: int var_asm asm("test");
 int var_asm asm("test");
+
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+// CHECK: enum __attribute__((ns_error_domain(MyErrorDomain))) MyErrorEnum {
+enum 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 284343.
MForster marked an inline comment as done.
MForster added a comment.

Fix nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-print-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -329,6 +329,8 @@
 // empty string but are then recorded as a nullptr.
 OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";
   else if (type == "TypeSourceInfo *")
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -15,3 +15,14 @@
 int fun_asm() asm("test");
 // CHECK: int var_asm asm("test");
 int var_asm asm("test");
+
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+// CHECK: enum __attribute__((ns_error_domain(MyErrorDomain))) 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

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

Continues to LGTM with your changes, but I did spot one tiny nit (no further 
reviews needed from me).




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5346
+  if (!isNSStringType(VD->getType(), S.Context)) {
+S.Diag(Loc, diag::err_nserrordomain_wrong_type) << DRE->getDecl();
+return;

Minor nit, you can pass `VD` instead of `DRE->getDecl()` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 284288.
MForster added a comment.

Fix mistake


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-print-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -329,6 +329,8 @@
 // empty string but are then recorded as a nullptr.
 OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";
   else if (type == "TypeSourceInfo *")
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -15,3 +15,14 @@
 int fun_asm() asm("test");
 // CHECK: int var_asm asm("test");
 int var_asm asm("test");
+
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+// CHECK: enum __attribute__((ns_error_domain(MyErrorDomain))) MyErrorEnum {
+enum 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.



Comment at: clang/test/AST/ast-print-attr.c:20
+
+2@interface NSString
+@end

Stray "2".




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 284286.
MForster added a comment.

Add an -ast-print test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-print-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -329,6 +329,8 @@
 // empty string but are then recorded as a nullptr.
 OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";
   else if (type == "TypeSourceInfo *")
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/test/AST/ast-print-attr.c
===
--- clang/test/AST/ast-print-attr.c
+++ clang/test/AST/ast-print-attr.c
@@ -15,3 +15,14 @@
 int fun_asm() asm("test");
 // CHECK: int var_asm asm("test");
 int var_asm asm("test");
+
+
+2@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+// CHECK: enum __attribute__((ns_error_domain(MyErrorDomain))) MyErrorEnum {
+enum 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:332
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";

I think this change is good, but if you wouldn't mind adding an ast dumping 
test (in the AST test directory) that exercises the change, I'd appreciate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 282168.
MForster added a comment.

Pretty-print VarDecl arguments correctly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -329,6 +329,8 @@
 // empty string but are then recorded as a nullptr.
 OS << "\" << (get" << getUpperName() << "() ? get" << getUpperName()
<< "()->getName() : \"\") << \"";
+  else if (type == "VarDecl *")
+OS << "\" << get" << getUpperName() << "()->getName() << \"";
   else if (type == "TypeSourceInfo *")
 OS << "\" << get" << getUpperName() << "().getAsString() << \"";
   else if (type == "ParamIdx")
Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

For context, this is the backported change, to be applied downstream before 
landing this review: https://github.com/apple/llvm-project/pull/1565


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281928.
MForster marked 5 inline comments as done.
MForster added a comment.

Fix patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace sema;
@@ -5322,6 +5326,30 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  auto *E = AL.getArgAsExpr(0);
+  auto Loc = E ? 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster marked an inline comment as done.
MForster added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));

aaron.ballman wrote:
> MForster wrote:
> > aaron.ballman wrote:
> > > MForster wrote:
> > > > aaron.ballman wrote:
> > > > > Shouldn't we also be validating that what we found is an NSString 
> > > > > that has the correct properties? (That doesn't seem like it should be 
> > > > > a change which risks breaking code based on what I understood from 
> > > > > Doug's description.)
> > > > > Shouldn't we also be validating that what we found is an NSString 
> > > > > that has the correct properties?
> > > > 
> > > > Is your suggestion to string-compare the name of the type?
> > > >>Shouldn't we also be validating that what we found is an NSString that 
> > > >>has the correct properties?
> > > > Is your suggestion to string-compare the name of the type?
> > > 
> > > You should be able to compare the `QualType` of the resulting `VarDecl` 
> > > against `ASTContext::getObjCNSStringType()` (accounting for qualifiers, 
> > > pointers, etc).
> > Turns out that this didn't work well. `ASTContext::getObjCNSStringType()` 
> > is only initialized if ObjC string literals are instantiated without an 
> > `NSString` type being defined. In our case there is an `NSString` type, 
> > because we need to declare a global variable of that type.
> > 
> > I've resorted to a string comparison after all.
> Well that's really unfortunate to learn, but good news: `isNSStringType()` is 
> in SemaDeclAttr.cpp and I hadn't noticed that before, so I think you can use 
> that instead, assuming that a mutable string is acceptable for the API. If 
> mutable strings are not acceptable, then I think we should add a new 
> parameter to `isNSStringType()` to handle it.
Nice find. Should have found this myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281927.
MForster added a comment.

Simplify code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/ns_error_enum.m

Index: clang/test/Analysis/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Analysis/ns_error_enum.m
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -verify %s
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument must be an identifier}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{ns_error_domain attribute only valid on enums, structs, and unions}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,39 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  if (!isa(D)) {
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
+<< S.getLangOpts().CPlusPlus;
+return;
+  }
+  IdentifierLoc *identLoc =
+  Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
+  if (!identLoc || !identLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation loc = Attr.getLoc();
+if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();
+
+S.Diag(loc, diag::err_nserrordomain_requires_identifier);
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(identLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7126,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9459,6 +9459,14 @@
 def err_nsreturns_retained_attribute_mismatch : Error<
   "overriding method has mismatched ns_returns_%select{not_retained|retained}0"
   " attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "
+  "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;
+def err_nserrordomain_invalid_decl : Error<
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;
+
 def warn_nsconsumed_attribute_mismatch : Warning<
   err_nsconsumed_attribute_mismatch.Text>, InGroup;
 def warn_nsreturns_retained_attribute_mismatch : Warning<
Index: clang/include/clang/Basic/AttrDocs.td

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from an almost NFC simplification.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));

MForster wrote:
> aaron.ballman wrote:
> > MForster wrote:
> > > aaron.ballman wrote:
> > > > Shouldn't we also be validating that what we found is an NSString that 
> > > > has the correct properties? (That doesn't seem like it should be a 
> > > > change which risks breaking code based on what I understood from Doug's 
> > > > description.)
> > > > Shouldn't we also be validating that what we found is an NSString that 
> > > > has the correct properties?
> > > 
> > > Is your suggestion to string-compare the name of the type?
> > >>Shouldn't we also be validating that what we found is an NSString that 
> > >>has the correct properties?
> > > Is your suggestion to string-compare the name of the type?
> > 
> > You should be able to compare the `QualType` of the resulting `VarDecl` 
> > against `ASTContext::getObjCNSStringType()` (accounting for qualifiers, 
> > pointers, etc).
> Turns out that this didn't work well. `ASTContext::getObjCNSStringType()` is 
> only initialized if ObjC string literals are instantiated without an 
> `NSString` type being defined. In our case there is an `NSString` type, 
> because we need to declare a global variable of that type.
> 
> I've resorted to a string comparison after all.
Well that's really unfortunate to learn, but good news: `isNSStringType()` is 
in SemaDeclAttr.cpp and I hadn't noticed that before, so I think you can use 
that instead, assuming that a mutable string is acceptable for the API. If 
mutable strings are not acceptable, then I think we should add a new parameter 
to `isNSStringType()` to handle it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281791.
MForster added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace sema;
@@ -5322,6 +5326,40 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  auto *E = AL.getArgAsExpr(0);
+  auto Loc = E ? E->getBeginLoc() : AL.getLoc();
+
+  auto *DRE = 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3650
   S.Diag(Field->getLocation(),
  diag::warn_transparent_union_attribute_field_size_align)
   << isSize << *Field << FieldBits;

gribozavr2 wrote:
> Please use spaces instead of tabs.
Sorry about that, fixed in 517fe058d42a1f937e14de4b61a5ac2ad326c850


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3650
   S.Diag(Field->getLocation(),
  diag::warn_transparent_union_attribute_field_size_align)
   << isSize << *Field << FieldBits;

Please use spaces instead of tabs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281550.
MForster added a comment.

Apply ClangTidy suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace sema;
@@ -5322,6 +5326,40 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  auto *E = AL.getArgAsExpr(0);
+  auto Loc = E ? E->getBeginLoc() : 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281546.
MForster added a comment.

Use declaration in diagnostics instead of name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace sema;
@@ -5322,6 +5326,40 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  auto *E = AL.getArgAsExpr(0);
+  auto Loc = E ? 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5342
+S.Diag(Loc, diag::err_nserrordomain_invalid_decl)
+<< 1 << DRE->getNameInfo().getName();
+return;

Just send the declaration into the diagnostic. See the recent D84656 for the 
rationale.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5358
+S.Diag(Loc, diag::err_nserrordomain_wrong_type)
+<< DRE->getNameInfo().getName();
+return;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281531.
MForster added a comment.

Rebase against master


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace sema;
@@ -5322,6 +5326,42 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  auto *E = AL.getArgAsExpr(0);
+  auto Loc = E ? E->getBeginLoc() : AL.getLoc();
+
+ 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));

aaron.ballman wrote:
> MForster wrote:
> > aaron.ballman wrote:
> > > Shouldn't we also be validating that what we found is an NSString that 
> > > has the correct properties? (That doesn't seem like it should be a change 
> > > which risks breaking code based on what I understood from Doug's 
> > > description.)
> > > Shouldn't we also be validating that what we found is an NSString that 
> > > has the correct properties?
> > 
> > Is your suggestion to string-compare the name of the type?
> >>Shouldn't we also be validating that what we found is an NSString that has 
> >>the correct properties?
> > Is your suggestion to string-compare the name of the type?
> 
> You should be able to compare the `QualType` of the resulting `VarDecl` 
> against `ASTContext::getObjCNSStringType()` (accounting for qualifiers, 
> pointers, etc).
Turns out that this didn't work well. `ASTContext::getObjCNSStringType()` is 
only initialized if ObjC string literals are instantiated without an `NSString` 
type being defined. In our case there is an `NSString` type, because we need to 
declare a global variable of that type.

I've resorted to a string comparison after all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 281525.
MForster marked 4 inline comments as done.
MForster added a comment.

Store the VarDecl instead of the identifier


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+
+typedef NSString *const NsErrorDomain;
+extern NsErrorDomain MyTypedefErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyTypedefErrorEnum, MyTypedefErrorDomain) {
+  MyTypedefErrFirst,
+  MyTypedefErrSecond,
+};
+
+extern char *const WrongErrorDomainType;
+enum __attribute__((ns_error_domain(WrongErrorDomainType))) MyWrongErrorDomainType { MyWrongErrorDomain };
+// expected-error@-1{{domain argument 'WrongErrorDomainType' does not point to an NSString constant}}
+
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{use of undeclared identifier 'InvalidDomain'}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument does not refer to global constant}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Mangle.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetBuiltins.h"
@@ -30,12 +31,15 @@
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ParsedAttr.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace sema;
@@ -5322,6 +5326,42 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  auto *E = 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));

MForster wrote:
> aaron.ballman wrote:
> > Shouldn't we also be validating that what we found is an NSString that has 
> > the correct properties? (That doesn't seem like it should be a change which 
> > risks breaking code based on what I understood from Doug's description.)
> > Shouldn't we also be validating that what we found is an NSString that has 
> > the correct properties?
> 
> Is your suggestion to string-compare the name of the type?
>>Shouldn't we also be validating that what we found is an NSString that has 
>>the correct properties?
> Is your suggestion to string-compare the name of the type?

You should be able to compare the `QualType` of the resulting `VarDecl` against 
`ASTContext::getObjCNSStringType()` (accounting for qualifiers, pointers, etc).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;

MForster wrote:
> aaron.ballman wrote:
> > MForster wrote:
> > > aaron.ballman wrote:
> > > > We're doing this lookup in the context of where the attribute is being 
> > > > used, but then creating the attribute with only the identifier and not 
> > > > the result of that lookup. This makes me a bit worried that when 
> > > > something goes to resolve that identifier to a variable later, it may 
> > > > get a different result because the lookup context will be different. Do 
> > > > you need to store the VarDecl on the semantic attribute, rather than 
> > > > the identifier?
> > > >We're doing this lookup in the context of where the attribute is being 
> > > >used, but then creating the attribute with only the identifier and not 
> > > >the result of that lookup. This makes me a bit worried that when 
> > > >something goes to resolve that identifier to a variable later, it may 
> > > >get a different result because the lookup context will be different. Do 
> > > >you need to store the VarDecl on the semantic attribute, rather than the 
> > > >identifier?
> > > 
> > > 
> > > When this gets imported into Swift, only the name of the identifier gets 
> > > used. I'm not quite sure why this was defined like this. This is another 
> > > thing where I would hope that @gribozavr2 or @milseman know more...
> > Based on the answer from @doug.gregor, I think this should be adding the 
> > result of the lookup to the semantic attribute and not the identifier (the 
> > identifier may not be unique, the VarDecl must be unique though).
> > Based on the answer from @doug.gregor, I think this should be adding the 
> > result of the lookup to the semantic attribute and not the identifier (the 
> > identifier may not be unique, the VarDecl must be unique though).
> 
> How are you suggesting to implement that? Change the argument to to be a 
> `DeclArgument` or `ExprArgument` instead of an `IdentifierArgument`?
>>Based on the answer from @doug.gregor, I think this should be adding the 
>>result of the lookup to the semantic attribute and not the identifier (the 
>>identifier may not be unique, the VarDecl must be unique though).
> How are you suggesting to implement that? Change the argument to to be a 
> DeclArgument or ExprArgument instead of an IdentifierArgument?

I think `DeclArgument` is probably the correct approach and you should be able 
to model it somewhat off the `cleanup` attribute. Otherwise, you can gin up a 
"fake" attribute argument. Those aren't arguments used by the parser or pretty 
printer, but are used to form the semantic attribute constructor to provide 
additional information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster marked 3 inline comments as done.
MForster added a comment.

Two clarifying questions...




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));

aaron.ballman wrote:
> Shouldn't we also be validating that what we found is an NSString that has 
> the correct properties? (That doesn't seem like it should be a change which 
> risks breaking code based on what I understood from Doug's description.)
> Shouldn't we also be validating that what we found is an NSString that has 
> the correct properties?

Is your suggestion to string-compare the name of the type?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;

aaron.ballman wrote:
> MForster wrote:
> > aaron.ballman wrote:
> > > We're doing this lookup in the context of where the attribute is being 
> > > used, but then creating the attribute with only the identifier and not 
> > > the result of that lookup. This makes me a bit worried that when 
> > > something goes to resolve that identifier to a variable later, it may get 
> > > a different result because the lookup context will be different. Do you 
> > > need to store the VarDecl on the semantic attribute, rather than the 
> > > identifier?
> > >We're doing this lookup in the context of where the attribute is being 
> > >used, but then creating the attribute with only the identifier and not the 
> > >result of that lookup. This makes me a bit worried that when something 
> > >goes to resolve that identifier to a variable later, it may get a 
> > >different result because the lookup context will be different. Do you need 
> > >to store the VarDecl on the semantic attribute, rather than the identifier?
> > 
> > 
> > When this gets imported into Swift, only the name of the identifier gets 
> > used. I'm not quite sure why this was defined like this. This is another 
> > thing where I would hope that @gribozavr2 or @milseman know more...
> Based on the answer from @doug.gregor, I think this should be adding the 
> result of the lookup to the semantic attribute and not the identifier (the 
> identifier may not be unique, the VarDecl must be unique though).
> Based on the answer from @doug.gregor, I think this should be adding the 
> result of the lookup to the semantic attribute and not the identifier (the 
> identifier may not be unique, the VarDecl must be unique though).

How are you suggesting to implement that? Change the argument to to be a 
`DeclArgument` or `ExprArgument` instead of an `IdentifierArgument`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5328
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation Loc = AL.getLoc();

Comments should be grammatical including punctuation (elsewhere as well).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5340
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),

`lookupResult` -> `Result` (or something else that matches the usual naming 
conventions).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));

Shouldn't we also be validating that what we found is an NSString that has the 
correct properties? (That doesn't seem like it should be a change which risks 
breaking code based on what I understood from Doug's description.)



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;

MForster wrote:
> aaron.ballman wrote:
> > We're doing this lookup in the context of where the attribute is being 
> > used, but then creating the attribute with only the identifier and not the 
> > result of that lookup. This makes me a bit worried that when something goes 
> > to resolve that identifier to a variable later, it may get a different 
> > result because the lookup context will be different. Do you need to store 
> > the VarDecl on the semantic attribute, rather than the identifier?
> >We're doing this lookup in the context of where the attribute is being used, 
> >but then creating the attribute with only the identifier and not the result 
> >of that lookup. This makes me a bit worried that when something goes to 
> >resolve that identifier to a variable later, it may get a different result 
> >because the lookup context will be different. Do you need to store the 
> >VarDecl on the semantic attribute, rather than the identifier?
> 
> 
> When this gets imported into Swift, only the name of the identifier gets 
> used. I'm not quite sure why this was defined like this. This is another 
> thing where I would hope that @gribozavr2 or @milseman know more...
Based on the answer from @doug.gregor, I think this should be adding the result 
of the lookup to the semantic attribute and not the identifier (the identifier 
may not be unique, the VarDecl must be unique though).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 280831.
MForster added a comment.

Rename test back to ns_error_enum.m


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.m

Index: clang/test/Sema/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.m
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be an identifier}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,35 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation Loc = AL.getLoc();
+if (const Expr *E = AL.getArgAsExpr(0))
+  Loc = E->getBeginLoc();
+
+S.Diag(Loc, diag::err_attribute_argument_n_type)
+<< AL << 1 << AANT_ArgumentIdentifier;
+
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< IdentLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7122,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 280829.
MForster marked an inline comment as done.
MForster added a comment.

Fix typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.c

Index: clang/test/Sema/ns_error_enum.c
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.c
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+// expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain(MyErrorDomain, MyErrorDomain))) TwoArgs { TwoArgsError };
+// expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be an identifier}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,35 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation Loc = AL.getLoc();
+if (const Expr *E = AL.getArgAsExpr(0))
+  Loc = E->getBeginLoc();
+
+S.Diag(Loc, diag::err_attribute_argument_n_type)
+<< AL << 1 << AANT_ArgumentIdentifier;
+
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< IdentLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7122,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+  

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster marked an inline comment as done.
MForster added inline comments.



Comment at: clang/test/Sema/ns_error_enum.c:25
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {

gribozavr2 wrote:
> const char * => NSString * const? You'd need to define a fake NSString type, 
> but that should be rather easy:
> 
> ```
> @interface NSString
> @end
> ```
> const char * => NSString * const?

Done.

Turns out that this makes the test incompatible with C and C++. Some more 
research makes me believe that this feature is only supposed to be used from 
ObjC(++). I removed the C/C++ RUN lines from this test. I will also send a 
follow-up change to turn this into an error in those languages. But I want to 
make it a separate change because of the potential for breakage in existing 
code.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 280828.
MForster marked 3 inline comments as done.
MForster added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.c

Index: clang/test/Sema/ns_error_enum.c
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.c
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+@interface NSString
+@end
+
+extern NSString *const MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructWithErrorDomain {};
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+enum __attribute__((ns_error_domain())) NoArg { NoArgError };
+  // expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+enum __attribute__((ns_error_domain())) TwoArgs { TwoArgsError };
+  // expected-error@-1{{'ns_error_domain' attribute takes one argument}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be an identifier}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,35 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation Loc = AL.getLoc();
+if (const Expr *E = AL.getArgAsExpr(0))
+  Loc = E->getBeginLoc();
+
+S.Diag(Loc, diag::err_attribute_argument_n_type)
+<< AL << 1 << AANT_ArgumentIdentifier;
+
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< IdentLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7122,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 280806.
MForster added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.c

Index: clang/test/Sema/ns_error_enum.c
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -verify %s -x c -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be an identifier}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,35 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation Loc = AL.getLoc();
+if (const Expr *E = AL.getArgAsExpr(0))
+  Loc = E->getBeginLoc();
+
+S.Diag(Loc, diag::err_attribute_argument_n_type)
+<< AL << 1 << AANT_ArgumentIdentifier;
+
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< IdentLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7122,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Basic/AttrDocs.td:3340
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {

const char * => NSString * const?



Comment at: clang/test/Sema/ns_error_enum.c:25
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {

const char * => NSString * const? You'd need to define a fake NSString type, 
but that should be rather easy:

```
@interface NSString
@end
```



Comment at: clang/test/Sema/ns_error_enum.c:30
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}

"MyStructWithErrorDomain" would be a better name, I think.



Comment at: clang/test/Sema/ns_error_enum.c:42
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be 
an identifier}}

Also a test for passing 0 or more than 1 argument to the attribute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

I have updated the attribute documentation to include the additional 
information provided by Doug. I think adding additional diagnostics would 
rather be separate changes.

I think I have addressed all remaining review comments.




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5344
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)

riccibruno wrote:
> Just a note that `LookupResult::getAsSingle` has tricky semantics (returns 
> null if the result is not exactly `LookupResultKind::Found`) and has been 
> (and still is) the source of many bugs in clang.
> 
> (Example: my favourite one is still the silly:
> ```
> struct S {
>   void not_overloaded();
>   enum { not_overloaded }; // error; redefinition of 'not_overloaded'
> 
>   void overloaded();
>   void overloaded(int);
>   enum { overloaded }; // clang is fine with this!
> };
> ```
> )
> 
> I don't know if it is a problem here or not though.
> Just a note that LookupResult::getAsSingle has tricky semantics [...]

Thanks for the advice, but I think this is not an issue here. The check should 
only succeed if there is a unique lookup result.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 280804.
MForster marked 8 inline comments as done.
MForster edited the summary of this revision.
MForster added a comment.

- Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.c

Index: clang/test/Sema/ns_error_enum.c
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -verify %s -x c -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be an identifier}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,35 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation Loc = AL.getLoc();
+if (const Expr *E = AL.getArgAsExpr(0))
+  Loc = E->getBeginLoc();
+
+S.Diag(Loc, diag::err_attribute_argument_n_type)
+<< AL << 1 << AANT_ArgumentIdentifier;
+
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< IdentLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7122,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84005#2172747 , @doug.gregor wrote:

> In D84005#2171982 , @MForster wrote:
>
> > @milseman, @doug.gregor, could you please help with the open questions on 
> > this review?
> >
> > Specifically:
> >
> > - Is `ns_error_domain` ever needed for something other than an enum?
>
>
> No, it only makes sense on enums.
>
> > - Why is this designed in the way it is (requiring an identifier for the 
> > domain, not allowing literals and then only using the name of the 
> > identifier from Swift)?
>
> It's codifying the design of NSError, which has been this way since... longer 
> than Clang has existed, and is independent of Swift. NSErrors have a domain 
> (identified by an NSString constant symbol, not a literal, for pointer 
> uniqueness and code size)  and a code (an integer, conventionally defined by 
> an enum). The two need to be used together, e.g., you create an NSError with 
> a domain and a code from that domain. This attribute finally ties those 
> things together at the source level.


Ah, thank you, that makes the design far more clear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Doug Gregor via Phabricator via cfe-commits]

doug.gregor added a comment.

In D84005#2171982 , @MForster wrote:

> @milseman, @doug.gregor, could you please help with the open questions on 
> this review?
>
> Specifically:
>
> - Is `ns_error_domain` ever needed for something other than an enum?


No, it only makes sense on enums.

> - Why is this designed in the way it is (requiring an identifier for the 
> domain, not allowing literals and then only using the name of the identifier 
> from Swift)?

It's codifying the design of NSError, which has been this way since... longer 
than Clang has existed, and is independent of Swift. NSErrors have a domain 
(identified by an NSString constant symbol, not a literal, for pointer 
uniqueness and code size)  and a code (an integer, conventionally defined by an 
enum). The two need to be used together, e.g., you create an NSError with a 
domain and a code from that domain. This attribute finally ties those things 
together at the source level.

This is leads to the answer to Aaron's question:

> Are there plans to use this attribute in Clang itself?

It would absolutely make sense to add some warnings if you've mismatched your 
domain and code when constructing an NSError (e.g., uses of 
https://developer.apple.com/documentation/foundation/nserror/1522782-errorwithdomain?language=objc)
 or even if when testing an error in an "if" statement (if checking both domain 
and code, make sure the code enumerator is from the same domain). I bet you'd 
catch some fiddly error-handling bugs this way.

> - Is it ok to make this attribute inheritable?

Sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84005#2171962 , @MForster wrote:

> In D84005#2162433 , @aaron.ballman 
> wrote:
>
> > It's a bit odd that this attribute has an AST node created for it but 
> > nothing is using that AST node elsewhere in the project. Are there other 
> > patches expected for making use of this attribute?
>
>
> It gets used from Swift.


Are there plans to use this attribute in Clang itself? If not, is there a way 
Swift can use attribute plugins instead of modifying Clang?




Comment at: clang/include/clang/Basic/AttrDocs.td:3330
+
+For example:
+

MForster wrote:
> aaron.ballman wrote:
> > I still feel like I'm lacking information about the domain. In the example, 
> > the domain is an uninitialized `const char *`, so how does this information 
> > get used by the attribute? Can I use any type of global I want? What about 
> > a literal?
> I won't claim that I understand this well. Maybe @gribozavr2 or @milseman can 
> explain this better. Literals are not allowed, however. The test explicitly 
> forbids that.
> Literals are not allowed, however. The test explicitly forbids that.

And I'm wondering why. For instance, attributes with enumeration arguments 
accept the enumerator as either an identifier or a string literal. I was sort 
of assuming that these domains are notionally like enumerators in that there is 
a list of domains, which is why I was asking about literals.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9454
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;

MForster wrote:
> aaron.ballman wrote:
> > I don't think this requires a custom diagnostic -- we can use 
> > `err_attribute_argument_n_type` instead.
> > I don't think this requires a custom diagnostic -- we can use 
> > `err_attribute_argument_n_type` instead.
> 
> 
> Done. Can you please confirm that `n` is 1-based?
Yes, the argument positions are 1-based.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5336
+  }
+  IdentifierLoc *identLoc =
+  Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;

aaron.ballman wrote:
> Variables should match the usual coding style conventions.
It looks like `loc` was missed and still need to be updated to `Loc`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5344
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)

Just a note that `LookupResult::getAsSingle` has tricky semantics (returns null 
if the result is not exactly `LookupResultKind::Found`) and has been (and still 
is) the source of many bugs in clang.

(Example: my favourite one is still the silly:
```
struct S {
  void not_overloaded();
  enum { not_overloaded }; // error; redefinition of 'not_overloaded'

  void overloaded();
  void overloaded(int);
  enum { overloaded }; // clang is fine with this!
};
```
)

I don't know if it is a problem here or not though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a subscriber: doug.gregor.
MForster added a comment.

@milseman, @doug.gregor, could you please help with the open questions on this 
review?

Specifically:

- Is `ns_error_domain` ever needed for something other than an enum?
- Why is this designed in the way it is (requiring an identifier for the 
domain, not allowing literals and then only using the name of the identifier 
from Swift)?
- Is it ok to make this attribute inheritable?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

In D84005#2162433 , @aaron.ballman 
wrote:

> It's a bit odd that this attribute has an AST node created for it but nothing 
> is using that AST node elsewhere in the project. Are there other patches 
> expected for making use of this attribute?


It gets used from Swift.




Comment at: clang/include/clang/Basic/AttrDocs.td:3330
+
+For example:
+

aaron.ballman wrote:
> I still feel like I'm lacking information about the domain. In the example, 
> the domain is an uninitialized `const char *`, so how does this information 
> get used by the attribute? Can I use any type of global I want? What about a 
> literal?
I won't claim that I understand this well. Maybe @gribozavr2 or @milseman can 
explain this better. Literals are not allowed, however. The test explicitly 
forbids that.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9454
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;

aaron.ballman wrote:
> I don't think this requires a custom diagnostic -- we can use 
> `err_attribute_argument_n_type` instead.
> I don't think this requires a custom diagnostic -- we can use 
> `err_attribute_argument_n_type` instead.


Done. Can you please confirm that `n` is 1-based?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;

aaron.ballman wrote:
> We're doing this lookup in the context of where the attribute is being used, 
> but then creating the attribute with only the identifier and not the result 
> of that lookup. This makes me a bit worried that when something goes to 
> resolve that identifier to a variable later, it may get a different result 
> because the lookup context will be different. Do you need to store the 
> VarDecl on the semantic attribute, rather than the identifier?
>We're doing this lookup in the context of where the attribute is being used, 
>but then creating the attribute with only the identifier and not the result of 
>that lookup. This makes me a bit worried that when something goes to resolve 
>that identifier to a variable later, it may get a different result because the 
>lookup context will be different. Do you need to store the VarDecl on the 
>semantic attribute, rather than the identifier?


When this gets imported into Swift, only the name of the identifier gets used. 
I'm not quite sure why this was defined like this. This is another thing where 
I would hope that @gribozavr2 or @milseman know more...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 280415.
MForster marked 15 inline comments as done.
MForster added a comment.
Herald added a reviewer: jdoerfert.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/ns_error_enum.c

Index: clang/test/Sema/ns_error_enum.c
===
--- /dev/null
+++ clang/test/Sema/ns_error_enum.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -verify %s -x c -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x objective-c
+// RUN: %clang_cc1 -verify %s -x objective-c++
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{'ns_error_domain' attribute only applies to enums}}
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{'ns_error_domain' attribute requires parameter 1 to be an identifier}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -80,6 +80,7 @@
 // CHECK-NEXT: MipsShortCall (SubjectMatchRule_function)
 // CHECK-NEXT: NSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NSConsumesSelf (SubjectMatchRule_objc_method)
+// CHECK-NEXT: NSErrorDomain (SubjectMatchRule_enum)
 // CHECK-NEXT: Naked (SubjectMatchRule_function)
 // CHECK-NEXT: NoBuiltin (SubjectMatchRule_function)
 // CHECK-NEXT: NoCommon (SubjectMatchRule_variable)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5322,6 +5322,35 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  IdentifierLoc *IdentLoc = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
+  if (!IdentLoc || !IdentLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation loc = AL.getLoc();
+if (const Expr *E = AL.getArgAsExpr(0))
+  loc = E->getBeginLoc();
+
+S.Diag(loc, diag::err_attribute_argument_n_type)
+<< AL << 1 << AANT_ArgumentIdentifier;
+
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< IdentLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7093,6 +7122,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

It's a bit odd that this attribute has an AST node created for it but nothing 
is using that AST node elsewhere in the project. Are there other patches 
expected for making use of this attribute?




Comment at: clang/include/clang/Basic/AttrDocs.td:3330
+
+For example:
+

I still feel like I'm lacking information about the domain. In the example, the 
domain is an uninitialized `const char *`, so how does this information get 
used by the attribute? Can I use any type of global I want? What about a 
literal?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9456
   " attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "

This diagnostic doesn't match the attribute definition (the subject list only 
lists enumerations). You should be able to remove this diagnostic entirely and 
just rely on the common attribute handler to diagnose incorrect subjects, 
though you should add an `ErrorDiag` to the subject list in Attr.td.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5330
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  if (!isa(D)) {

Please don't use `Attr` as a declaration identifier, it's the name of a type 
already and that confuses some editors.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5331
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  if (!isa(D)) {
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)

This code should be removed, the common attribute handler already diagnoses 
incorrect subjects (and this doesn't match the interface in Attr.td anyway).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5336
+  }
+  IdentifierLoc *identLoc =
+  Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;

Variables should match the usual coding style conventions.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;

We're doing this lookup in the context of where the attribute is being used, 
but then creating the attribute with only the identifier and not the result of 
that lookup. This makes me a bit worried that when something goes to resolve 
that identifier to a variable later, it may get a different result because the 
lookup context will be different. Do you need to store the VarDecl on the 
semantic attribute, rather than the identifier?



Comment at: clang/test/Analysis/ns_error_enum.c:1
+// RUN: %clang_cc1 -verify %s -x c -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base

The test should still be moved out of Analysis and into Sema.



Comment at: clang/test/Analysis/ns_error_enum.c:3
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x objective-c
+

One more run line for `objective-c++`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster marked an inline comment as done.
MForster added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

MForster wrote:
> aaron.ballman wrote:
> > gribozavr2 wrote:
> > > aaron.ballman wrote:
> > > > MForster wrote:
> > > > > gribozavr2 wrote:
> > > > > > Could we try to add a list of subjects here? It seems like it is a 
> > > > > > type-only attribute, and most likely enum-only.
> > > > > > 
> > > > > > let Subjects = SubjectList<[Enum]>;
> > > > > @milseman, could you comment on this? 
> > > > > 
> > > > > In the meantime I've added the restriction. Obviously this makes the 
> > > > > tests fail. I will also test this change against the Swift unit tests.
> > > > FWIW, this is not a attribute; it's a declaration attribute.
> > > > 
> > > > Is there a reason it's not inheritable?
> > > > 
> > > > I assume it's not getting a Clang spelling because Objective-C isn't 
> > > > tracking C2x yet? (Though that spelling still seems useful to 
> > > > Objective-C++ users in general for these NS attributes.)
> > > > FWIW, this is not a attribute; it's a declaration attribute.
> > > 
> > > Sorry, yes, of course I meant to say "declaration attribute".
> > > 
> > > > Is there a reason it's not inheritable?
> > > 
> > > Good observation, I think it should be.
> > > 
> > > > I assume it's not getting a Clang spelling because Objective-C isn't 
> > > > tracking C2x yet?
> > > 
> > > Cocoa users are expected to use the `NS_*` macros instead of using the 
> > > attribute directly, so even if a C2x spelling was an option (IDK if it 
> > > is), there would be very limited use for it.
> > > Cocoa users are expected to use the NS_* macros instead of using the 
> > > attribute directly, so even if a C2x spelling was an option (IDK if it 
> > > is), there would be very limited use for it.
> > 
> > Okay, that's reasonable, thanks!
> > Is there a reason it's not inheritable?
> 
> I made it inheritable. @milseman, any comment on whether that is appropriate? 
>> Could we try to add a list of subjects here? It seems like it is a type-only 
>> attribute, and most likely enum-only.
>>
>> let Subjects = SubjectList<[Enum]>;
>
> @milseman, could you comment on this?
>
> In the meantime I've added the restriction. Obviously this makes the tests 
> fail. I will also test this change against the Swift unit tests.

FWIW, no failures in the Swift tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

aaron.ballman wrote:
> gribozavr2 wrote:
> > aaron.ballman wrote:
> > > MForster wrote:
> > > > gribozavr2 wrote:
> > > > > Could we try to add a list of subjects here? It seems like it is a 
> > > > > type-only attribute, and most likely enum-only.
> > > > > 
> > > > > let Subjects = SubjectList<[Enum]>;
> > > > @milseman, could you comment on this? 
> > > > 
> > > > In the meantime I've added the restriction. Obviously this makes the 
> > > > tests fail. I will also test this change against the Swift unit tests.
> > > FWIW, this is not a attribute; it's a declaration attribute.
> > > 
> > > Is there a reason it's not inheritable?
> > > 
> > > I assume it's not getting a Clang spelling because Objective-C isn't 
> > > tracking C2x yet? (Though that spelling still seems useful to 
> > > Objective-C++ users in general for these NS attributes.)
> > > FWIW, this is not a attribute; it's a declaration attribute.
> > 
> > Sorry, yes, of course I meant to say "declaration attribute".
> > 
> > > Is there a reason it's not inheritable?
> > 
> > Good observation, I think it should be.
> > 
> > > I assume it's not getting a Clang spelling because Objective-C isn't 
> > > tracking C2x yet?
> > 
> > Cocoa users are expected to use the `NS_*` macros instead of using the 
> > attribute directly, so even if a C2x spelling was an option (IDK if it is), 
> > there would be very limited use for it.
> > Cocoa users are expected to use the NS_* macros instead of using the 
> > attribute directly, so even if a C2x spelling was an option (IDK if it is), 
> > there would be very limited use for it.
> 
> Okay, that's reasonable, thanks!
> Is there a reason it's not inheritable?

I made it inheritable. @milseman, any comment on whether that is appropriate? 



Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

aaron.ballman wrote:
> gribozavr2 wrote:
> > aaron.ballman wrote:
> > > MForster wrote:
> > > > gribozavr2 wrote:
> > > > > This file is a `.m` -- any specific reason? I'd call it `.c` and run 
> > > > > the test in C, Objective-C, and C++ modes (enums might work slightly 
> > > > > differently, the name lookup functionality might work differently).
> > > > The test doesn't compile in C or C++ (`non-defining declaration of 
> > > > enumeration with a fixed underlying type is only permitted as a 
> > > > standalone declaration; missing list of enumerators?`). Not sure if 
> > > > it's worth adapting.
> > > Enums with fixed underlying types exist in C++ and C, so I was expecting 
> > > the attribute to work there. If the attribute isn't supported in these 
> > > languages, should the attribute be tied to a language mode?
> > There are Apple SDK headers that parse in all language modes (C, 
> > Objective-C, C++, Objective-C++), so I think it is quite important to test 
> > this feature in all modes. I suspect the reason for the error is that 
> > different language modes require a slightly different macro definition.
> > There are Apple SDK headers that parse in all language modes (C, 
> > Objective-C, C++, Objective-C++), so I think it is quite important to test 
> > this feature in all modes.
> 
> In that case, I definitely agree. This should have multiple RUN lines to test 
> the various language modes.
>>There are Apple SDK headers that parse in all language modes (C, Objective-C, 
>>C++, Objective-C++), so I think it is quite important to test this feature in 
>>all modes.
>In that case, I definitely agree. This should have multiple RUN lines to test 
>the various language modes.

So, it seems that the error message is quite new. I suspect that the Apple SDK 
headers have not yet been updated. At least I'm getting the same error messages 
when running the test with the (rather elaborate) original definitions of 
`CF_ENUM` and `NS_ERROR_ENUM`. 

For now I have just disabled the diagnostic for C and C++ 
(`-Wno-elaborated-enum-base`). For ObjC it is disabled anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 279228.
MForster marked 13 inline comments as done.
MForster added a comment.

- Run test also in C and C++ mode
- Address more review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/ns_error_enum.c

Index: clang/test/Analysis/ns_error_enum.c
===
--- /dev/null
+++ clang/test/Analysis/ns_error_enum.c
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -verify %s -x c -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x objective-c
+
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument must be an identifier}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{ns_error_domain attribute only valid on enums, structs, and unions}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5327,6 +5327,39 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  if (!isa(D)) {
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
+<< S.getLangOpts().CPlusPlus;
+return;
+  }
+  IdentifierLoc *identLoc =
+  Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
+  if (!identLoc || !identLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation loc = Attr.getLoc();
+if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();
+
+S.Diag(loc, diag::err_nserrordomain_requires_identifier);
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(identLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7098,6 +7131,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9453,6 +9453,14 @@
 def err_nsreturns_retained_attribute_mismatch : Error<
   "overriding method has mismatched ns_returns_%select{not_retained|retained}0"
   " attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "
+  "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;
+def err_nserrordomain_invalid_decl : Error<
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

gribozavr2 wrote:
> aaron.ballman wrote:
> > MForster wrote:
> > > gribozavr2 wrote:
> > > > Could we try to add a list of subjects here? It seems like it is a 
> > > > type-only attribute, and most likely enum-only.
> > > > 
> > > > let Subjects = SubjectList<[Enum]>;
> > > @milseman, could you comment on this? 
> > > 
> > > In the meantime I've added the restriction. Obviously this makes the 
> > > tests fail. I will also test this change against the Swift unit tests.
> > FWIW, this is not a attribute; it's a declaration attribute.
> > 
> > Is there a reason it's not inheritable?
> > 
> > I assume it's not getting a Clang spelling because Objective-C isn't 
> > tracking C2x yet? (Though that spelling still seems useful to Objective-C++ 
> > users in general for these NS attributes.)
> > FWIW, this is not a attribute; it's a declaration attribute.
> 
> Sorry, yes, of course I meant to say "declaration attribute".
> 
> > Is there a reason it's not inheritable?
> 
> Good observation, I think it should be.
> 
> > I assume it's not getting a Clang spelling because Objective-C isn't 
> > tracking C2x yet?
> 
> Cocoa users are expected to use the `NS_*` macros instead of using the 
> attribute directly, so even if a C2x spelling was an option (IDK if it is), 
> there would be very limited use for it.
> Cocoa users are expected to use the NS_* macros instead of using the 
> attribute directly, so even if a C2x spelling was an option (IDK if it is), 
> there would be very limited use for it.

Okay, that's reasonable, thanks!



Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

gribozavr2 wrote:
> aaron.ballman wrote:
> > MForster wrote:
> > > gribozavr2 wrote:
> > > > This file is a `.m` -- any specific reason? I'd call it `.c` and run 
> > > > the test in C, Objective-C, and C++ modes (enums might work slightly 
> > > > differently, the name lookup functionality might work differently).
> > > The test doesn't compile in C or C++ (`non-defining declaration of 
> > > enumeration with a fixed underlying type is only permitted as a 
> > > standalone declaration; missing list of enumerators?`). Not sure if it's 
> > > worth adapting.
> > Enums with fixed underlying types exist in C++ and C, so I was expecting 
> > the attribute to work there. If the attribute isn't supported in these 
> > languages, should the attribute be tied to a language mode?
> There are Apple SDK headers that parse in all language modes (C, Objective-C, 
> C++, Objective-C++), so I think it is quite important to test this feature in 
> all modes. I suspect the reason for the error is that different language 
> modes require a slightly different macro definition.
> There are Apple SDK headers that parse in all language modes (C, Objective-C, 
> C++, Objective-C++), so I think it is quite important to test this feature in 
> all modes.

In that case, I definitely agree. This should have multiple RUN lines to test 
the various language modes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

aaron.ballman wrote:
> MForster wrote:
> > gribozavr2 wrote:
> > > Could we try to add a list of subjects here? It seems like it is a 
> > > type-only attribute, and most likely enum-only.
> > > 
> > > let Subjects = SubjectList<[Enum]>;
> > @milseman, could you comment on this? 
> > 
> > In the meantime I've added the restriction. Obviously this makes the tests 
> > fail. I will also test this change against the Swift unit tests.
> FWIW, this is not a attribute; it's a declaration attribute.
> 
> Is there a reason it's not inheritable?
> 
> I assume it's not getting a Clang spelling because Objective-C isn't tracking 
> C2x yet? (Though that spelling still seems useful to Objective-C++ users in 
> general for these NS attributes.)
> FWIW, this is not a attribute; it's a declaration attribute.

Sorry, yes, of course I meant to say "declaration attribute".

> Is there a reason it's not inheritable?

Good observation, I think it should be.

> I assume it's not getting a Clang spelling because Objective-C isn't tracking 
> C2x yet?

Cocoa users are expected to use the `NS_*` macros instead of using the 
attribute directly, so even if a C2x spelling was an option (IDK if it is), 
there would be very limited use for it.



Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

aaron.ballman wrote:
> MForster wrote:
> > gribozavr2 wrote:
> > > This file is a `.m` -- any specific reason? I'd call it `.c` and run the 
> > > test in C, Objective-C, and C++ modes (enums might work slightly 
> > > differently, the name lookup functionality might work differently).
> > The test doesn't compile in C or C++ (`non-defining declaration of 
> > enumeration with a fixed underlying type is only permitted as a standalone 
> > declaration; missing list of enumerators?`). Not sure if it's worth 
> > adapting.
> Enums with fixed underlying types exist in C++ and C, so I was expecting the 
> attribute to work there. If the attribute isn't supported in these languages, 
> should the attribute be tied to a language mode?
There are Apple SDK headers that parse in all language modes (C, Objective-C, 
C++, Objective-C++), so I think it is quite important to test this feature in 
all modes. I suspect the reason for the error is that different language modes 
require a slightly different macro definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9450
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "
+  "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;

ns_error_domain -> 'ns_error_domain' (with the single quotes around it, because 
it's syntax).



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9454
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;

I don't think this requires a custom diagnostic -- we can use 
`err_attribute_argument_n_type` instead.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5332
   if (!isa(D)) {
-S.Diag(D->getLocStart(), diag::err_nserrordomain_not_tagdecl)
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
 << S.getLangOpts().CPlusPlus;

aaron.ballman wrote:
> Where is this error defined?
> Where is this error defined?

Now I found it, it was dropped in the version of the review I was looking at. 
:-)



Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

I think this test belongs in Sema, not in Analysis (nothing about the feature 
is specific to static analysis).

Also, missing Sema tests that the attribute appertains to the correct subject, 
accepts the correct number and types of arguments, etc.

I'd also appreciate seeing tests of the edge cases, like a locally-defined 
enumeration, a forward declaration of an enumeration, etc.



Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

MForster wrote:
> gribozavr2 wrote:
> > This file is a `.m` -- any specific reason? I'd call it `.c` and run the 
> > test in C, Objective-C, and C++ modes (enums might work slightly 
> > differently, the name lookup functionality might work differently).
> The test doesn't compile in C or C++ (`non-defining declaration of 
> enumeration with a fixed underlying type is only permitted as a standalone 
> declaration; missing list of enumerators?`). Not sure if it's worth adapting.
Enums with fixed underlying types exist in C++ and C, so I was expecting the 
attribute to work there. If the attribute isn't supported in these languages, 
should the attribute be tied to a language mode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

In D84005#2158029 , @aaron.ballman 
wrote:

> FWIW, this is *really* hard to review because it's not a diff against the 
> trunk and so it's not immediately clear what the actual changes are.
>
> The change is missing all of its test coverage.


I'm sorry. This was me struggling with arcanist. It should be a diff against 
trunk now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

In D84005#2158019 , @gribozavr2 wrote:

> Did your latest update unintentionally drop the test file 
> `clang/test/Analysis/ns_error_enum.m`?


I'm struggling with arcanist :-). Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 278728.
MForster marked an inline comment as not done.
MForster added a comment.

Fix diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/ns_error_enum.m

Index: clang/test/Analysis/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Analysis/ns_error_enum.m
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -verify %s
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument must be an identifier}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{ns_error_domain attribute only valid on enums, structs, and unions}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5327,6 +5327,39 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  if (!isa(D)) {
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
+<< S.getLangOpts().CPlusPlus;
+return;
+  }
+  IdentifierLoc *identLoc =
+  Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
+  if (!identLoc || !identLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation loc = Attr.getLoc();
+if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();
+
+S.Diag(loc, diag::err_nserrordomain_requires_identifier);
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(identLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7098,6 +7131,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9446,6 +9446,14 @@
 def err_nsreturns_retained_attribute_mismatch : Error<
   "overriding method has mismatched ns_returns_%select{not_retained|retained}0"
   " attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "
+  "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;
+def err_nserrordomain_invalid_decl : Error<
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;
+
 def warn_nsconsumed_attribute_mismatch : Warning<
   err_nsconsumed_attribute_mismatch.Text>, InGroup;
 def warn_nsreturns_retained_attribute_mismatch : Warning<
Index: 

[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

FWIW, this is *really* hard to review because it's not a diff against the trunk 
and so it's not immediately clear what the actual changes are.

The change is missing all of its test coverage.




Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

MForster wrote:
> gribozavr2 wrote:
> > Could we try to add a list of subjects here? It seems like it is a 
> > type-only attribute, and most likely enum-only.
> > 
> > let Subjects = SubjectList<[Enum]>;
> @milseman, could you comment on this? 
> 
> In the meantime I've added the restriction. Obviously this makes the tests 
> fail. I will also test this change against the Swift unit tests.
FWIW, this is not a attribute; it's a declaration attribute.

Is there a reason it's not inheritable?

I assume it's not getting a Clang spelling because Objective-C isn't tracking 
C2x yet? (Though that spelling still seems useful to Objective-C++ users in 
general for these NS attributes.)



Comment at: clang/include/clang/Basic/AttrDocs.td:3317
+def NSErrorDomainDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{

gribozavr2 wrote:
> Shouldn't the category be DocCatType since it is a type attribute?
This is not a type attribute. It should be set to `DocCatDecl`.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5330
 
-static void handleNSErrorDomain(Sema , Decl *D, const AttributeList ) {
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
   if (!isa(D)) {

Please do not name the parameter with the same name as a type.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5331
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
   if (!isa(D)) {
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)

This shouldn't be necessary as the common attribute handler checks subjects. 
Also, it's the wrong subject (this would allow things like putting the 
attribute onto a struct).



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5332
   if (!isa(D)) {
-S.Diag(D->getLocStart(), diag::err_nserrordomain_not_tagdecl)
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
 << S.getLangOpts().CPlusPlus;

Where is this error defined?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5336
   }
   IdentifierLoc *identLoc =
   Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;

Doesn't match the usual naming conventions. Same issue elsewhere in the patch.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5341
 SourceLocation loc = Attr.getLoc();
 if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();

```
if (const Expr *E = Attr.getArgAsExpr(0))
  loc = E->getBeginLoc();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

Did your latest update unintentionally drop the test file 
`clang/test/Analysis/ns_error_enum.m`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster marked an inline comment as done and an inline comment as not done.
MForster added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

gribozavr2 wrote:
> Could we try to add a list of subjects here? It seems like it is a type-only 
> attribute, and most likely enum-only.
> 
> let Subjects = SubjectList<[Enum]>;
@milseman, could you comment on this? 

In the meantime I've added the restriction. Obviously this makes the tests 
fail. I will also test this change against the Swift unit tests.



Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

gribozavr2 wrote:
> This file is a `.m` -- any specific reason? I'd call it `.c` and run the test 
> in C, Objective-C, and C++ modes (enums might work slightly differently, the 
> name lookup functionality might work differently).
The test doesn't compile in C or C++ (`non-defining declaration of enumeration 
with a fixed underlying type is only permitted as a standalone declaration; 
missing list of enumerators?`). Not sure if it's worth adapting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster updated this revision to Diff 278724.
MForster marked 4 inline comments as done.
MForster added a comment.

- Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5327,9 +5327,9 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
-static void handleNSErrorDomain(Sema , Decl *D, const AttributeList ) {
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
   if (!isa(D)) {
-S.Diag(D->getLocStart(), diag::err_nserrordomain_not_tagdecl)
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
 << S.getLangOpts().CPlusPlus;
 return;
   }
@@ -5339,7 +5339,7 @@
 // Try to locate the argument directly
 SourceLocation loc = Attr.getLoc();
 if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
-  loc = Attr.getArgAsExpr(0)->getLocStart();
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();
 
 S.Diag(loc, diag::err_nserrordomain_requires_identifier);
 return;
@@ -5357,8 +5357,7 @@
   }
 
   D->addAttr(::new (S.Context)
- NSErrorDomainAttr(Attr.getRange(), S.Context, identLoc->Ident,
-   Attr.getAttributeSpellingListIndex()));
+ NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
 }
 
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
@@ -7132,8 +7131,8 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
-  case AttributeList::AT_NSErrorDomain:
-handleNSErrorDomain(S, D, Attr);
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
 break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions];
+  let Subjects = SubjectList<[Enum]>;
   let Args = [IdentifierArgument<"ErrorDomain">];
   let Documentation = [NSErrorDomainDocs];
 }


Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5327,9 +5327,9 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
-static void handleNSErrorDomain(Sema , Decl *D, const AttributeList ) {
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
   if (!isa(D)) {
-S.Diag(D->getLocStart(), diag::err_nserrordomain_not_tagdecl)
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
 << S.getLangOpts().CPlusPlus;
 return;
   }
@@ -5339,7 +5339,7 @@
 // Try to locate the argument directly
 SourceLocation loc = Attr.getLoc();
 if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
-  loc = Attr.getArgAsExpr(0)->getLocStart();
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();
 
 S.Diag(loc, diag::err_nserrordomain_requires_identifier);
 return;
@@ -5357,8 +5357,7 @@
   }
 
   D->addAttr(::new (S.Context)
- NSErrorDomainAttr(Attr.getRange(), S.Context, identLoc->Ident,
-   Attr.getAttributeSpellingListIndex()));
+ NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
 }
 
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
@@ -7132,8 +7131,8 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
-  case AttributeList::AT_NSErrorDomain:
-handleNSErrorDomain(S, D, Attr);
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
 break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions];
+  let Subjects = SubjectList<[Enum]>;
   let Args = [IdentifierArgument<"ErrorDomain">];
   let Documentation = [NSErrorDomainDocs];
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84005: Introduce ns_error_domain attribute.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1860
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Args = [IdentifierArgument<"ErrorDomain">];

Could we try to add a list of subjects here? It seems like it is a type-only 
attribute, and most likely enum-only.

let Subjects = SubjectList<[Enum]>;



Comment at: clang/include/clang/Basic/AttrDocs.td:3317
+def NSErrorDomainDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{

Shouldn't the category be DocCatType since it is a type attribute?



Comment at: clang/include/clang/Basic/AttrDocs.td:3319
+  let Content = [{
+The ``ns_error_domain`` attribute indicates a global constant representing the 
error domain.
+  }];

I think we should try to expand the docs. For example:

In Cocoa frameworks in Objective-C, one can group related error codes in enums, 
and categorize these enums with error domains.

The ``ns_error_domain`` attribute specifies the error domain that error codes 
belong to. This attribute is attached to enums that describe error codes.

This metadata is useful for documentation purposes, for static analysis, and 
for improving interoperability between Objective-C and Swift. It is not used 
for code generation in Objective-C.

For example:

(insert an example from tests)





Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+

This file is a `.m` -- any specific reason? I'd call it `.c` and run the test 
in C, Objective-C, and C++ modes (enums might work slightly differently, the 
name lookup functionality might work differently).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005



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

[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster created this revision.
MForster added reviewers: gribozavr2, milseman.
Herald added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is chery-picked from 
https://github.com/llvm/llvm-project-staging/commit/a14779f504b02ad0e4dbc39d6d10cadc7ed4cfd0
and adapted to updated Clang APIs.

ns_error_domain can be used by, e.g. NS_ERROR_ENUM, in order to
identify a global declaration representing the domain constant.

Introduces the attribute, Sema handling, diagnostics, and test case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84005

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Analysis/ns_error_enum.m

Index: clang/test/Analysis/ns_error_enum.m
===
--- /dev/null
+++ clang/test/Analysis/ns_error_enum.m
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -verify %s
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+	MyErrFirst,
+	MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+	// expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+	MyErrFirstInvalid,
+	MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument must be an identifier}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{ns_error_domain attribute only valid on enums, structs, and unions}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -5327,6 +5327,39 @@
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema , Decl *D, const ParsedAttr ) {
+  if (!isa(D)) {
+S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
+<< S.getLangOpts().CPlusPlus;
+return;
+  }
+  IdentifierLoc *identLoc =
+  Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
+  if (!identLoc || !identLoc->Ident) {
+// Try to locate the argument directly
+SourceLocation loc = Attr.getLoc();
+if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
+  loc = Attr.getArgAsExpr(0)->getBeginLoc();
+
+S.Diag(loc, diag::err_nserrordomain_requires_identifier);
+return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(identLoc->Ident),
+SourceLocation(),
+Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+  !lookupResult.getAsSingle()) {
+S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+<< identLoc->Ident;
+return;
+  }
+
+  D->addAttr(::new (S.Context)
+ NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema , Decl *D, const ParsedAttr ) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
@@ -7098,6 +7131,9 @@
   case ParsedAttr::AT_ObjCBoxable:
 handleObjCBoxable(S, D, AL);
 break;
+  case ParsedAttr::AT_NSErrorDomain:
+handleNSErrorDomain(S, D, AL);
+break;
   case ParsedAttr::AT_CFAuditedTransfer:
 handleSimpleAttributeWithExclusions(S, D, AL);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9446,6 +9446,14 @@
 def err_nsreturns_retained_attribute_mismatch : Error<
   "overriding method has mismatched ns_returns_%select{not_retained|retained}0"
   " attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "
+  "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;
+def err_nserrordomain_invalid_decl