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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits