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: > > aaron.ballman wrote: > > > 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.) > > > FWIW, this is not a attribute; it's a declaration attribute. > > > > Sorry, yes, of course I meant to say "declaration attribute". > > > > > Is there a reason it's not inheritable? > > > > Good observation, I think it should be. > > > > > I assume it's not getting a Clang spelling because Objective-C isn't > > > tracking C2x yet? > > > > Cocoa users are expected to use the `NS_*` macros instead of using the > > attribute directly, so even if a C2x spelling was an option (IDK if it is), > > there would be very limited use for it. > > Cocoa users are expected to use the NS_* macros instead of using the > > attribute directly, so even if a C2x spelling was an option (IDK if it is), > > there would be very limited use for it. > > Okay, that's reasonable, thanks! > Is there a reason it's not inheritable? I made it inheritable. @milseman, any comment on whether that is appropriate? ================ Comment at: clang/test/Analysis/ns_error_enum.m:1 +// RUN: %clang_cc1 -verify %s + ---------------- aaron.ballman wrote: > gribozavr2 wrote: > > aaron.ballman wrote: > > > 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? > > There are Apple SDK headers that parse in all language modes (C, > > Objective-C, C++, Objective-C++), so I think it is quite important to test > > this feature in all modes. I suspect the reason for the error is that > > different language modes require a slightly different macro definition. > > There are Apple SDK headers that parse in all language modes (C, > > Objective-C, C++, Objective-C++), so I think it is quite important to test > > this feature in all modes. > > In that case, I definitely agree. This should have multiple RUN lines to test > the various language modes. >>There are Apple SDK headers that parse in all language modes (C, Objective-C, >>C++, Objective-C++), so I think it is quite important to test this feature in >>all modes. >In that case, I definitely agree. This should have multiple RUN lines to test >the various language modes. So, it seems that the error message is quite new. I suspect that the Apple SDK headers have not yet been updated. At least I'm getting the same error messages when running the test with the (rather elaborate) original definitions of `CF_ENUM` and `NS_ERROR_ENUM`. For now I have just disabled the diagnostic for C and C++ (`-Wno-elaborated-enum-base`). For ObjC it is disabled anyway. 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