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