[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:5328
+  if (!IdentLoc || !IdentLoc->Ident) {
+    // Try to locate the argument directly
+    SourceLocation Loc = AL.getLoc();
----------------
Comments should be grammatical including punctuation (elsewhere as well).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5340
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(IdentLoc->Ident),
+                            SourceLocation(),
----------------
`lookupResult` -> `Result` (or something else that matches the usual naming conventions).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5350
+
+  D->addAttr(::new (S.Context)
+                 NSErrorDomainAttr(S.Context, AL, IdentLoc->Ident));
----------------
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.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005