[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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/

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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)) { +

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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/

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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));

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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;

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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: > > >

[PATCH] D84005: Introduce ns_error_domain attribute.

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/

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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));

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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 * =>

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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) {

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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/

[PATCH] D84005: Introduce ns_error_domain attribute.

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? > > >

[PATCH] D84005: Introduce ns_error_domain attribute.

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,

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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/

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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: > >

[PATCH] D84005: Introduce ns_error_domain attribute.

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/

[PATCH] D84005: Introduce ns_error_domain attribute.

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: > >

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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">;

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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 =

[PATCH] D84005: Introduce ns_error_domain attribute.

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:

[PATCH] D84005: Introduce ns_error_domain attribute.

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

[PATCH] D84005: Introduce ns_error_domain attribute.

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