[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 &S, Decl *D, const ParsedAttr &Attr) {
+  if (!isa<TagDecl>(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 &S, Decl *D, const ParsedAttr &Attr) {
+  if (!isa<TagDecl>(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