[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<TagDecl>(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