[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+    S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+        << identLoc->Ident;
+    return;
----------------
MForster wrote:
> aaron.ballman wrote:
> > MForster wrote:
> > > aaron.ballman wrote:
> > > > We're doing this lookup in the context of where the attribute is being used, but then creating the attribute with only the identifier and not the result of that lookup. This makes me a bit worried that when something goes to resolve that identifier to a variable later, it may get a different result because the lookup context will be different. Do you need to store the VarDecl on the semantic attribute, rather than the identifier?
> > > >We're doing this lookup in the context of where the attribute is being used, but then creating the attribute with only the identifier and not the result of that lookup. This makes me a bit worried that when something goes to resolve that identifier to a variable later, it may get a different result because the lookup context will be different. Do you need to store the VarDecl on the semantic attribute, rather than the identifier?
> > > 
> > > 
> > > When this gets imported into Swift, only the name of the identifier gets used. I'm not quite sure why this was defined like this. This is another thing where I would hope that @gribozavr2 or @milseman know more...
> > Based on the answer from @doug.gregor, I think this should be adding the result of the lookup to the semantic attribute and not the identifier (the identifier may not be unique, the VarDecl must be unique though).
> > Based on the answer from @doug.gregor, I think this should be adding the result of the lookup to the semantic attribute and not the identifier (the identifier may not be unique, the VarDecl must be unique though).
> 
> How are you suggesting to implement that? Change the argument to to be a `DeclArgument` or `ExprArgument` instead of an `IdentifierArgument`?
>>Based on the answer from @doug.gregor, I think this should be adding the result of the lookup to the semantic attribute and not the identifier (the identifier may not be unique, the VarDecl must be unique though).
> How are you suggesting to implement that? Change the argument to to be a DeclArgument or ExprArgument instead of an IdentifierArgument?

I think `DeclArgument` is probably the correct approach and you should be able to model it somewhat off the `cleanup` attribute. Otherwise, you can gin up a "fake" attribute argument. Those aren't arguments used by the parser or pretty printer, but are used to form the semantic attribute constructor to provide additional information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005