[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

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 wrote:
> > aaron.ballman wrote:
> > > MForster wrote:
> > > > aaron.ballman wrote:
> > > > > Shouldn't we also be validating that what we found is an NSString that has the correct properties? (That doesn't seem like it should be a change which risks breaking code based on what I understood from Doug's description.)
> > > > > Shouldn't we also be validating that what we found is an NSString that has the correct properties?
> > > > 
> > > > Is your suggestion to string-compare the name of the type?
> > > >>Shouldn't we also be validating that what we found is an NSString that has the correct properties?
> > > > Is your suggestion to string-compare the name of the type?
> > > 
> > > You should be able to compare the `QualType` of the resulting `VarDecl` against `ASTContext::getObjCNSStringType()` (accounting for qualifiers, pointers, etc).
> > Turns out that this didn't work well. `ASTContext::getObjCNSStringType()` is only initialized if ObjC string literals are instantiated without an `NSString` type being defined. In our case there is an `NSString` type, because we need to declare a global variable of that type.
> > 
> > I've resorted to a string comparison after all.
> Well that's really unfortunate to learn, but good news: `isNSStringType()` is in SemaDeclAttr.cpp and I hadn't noticed that before, so I think you can use that instead, assuming that a mutable string is acceptable for the API. If mutable strings are not acceptable, then I think we should add a new parameter to `isNSStringType()` to handle it.
Nice find. Should have found this myself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84005/new/

https://reviews.llvm.org/D84005