[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

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