[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

In D84005#2162433 <https://reviews.llvm.org/D84005#2162433>, @aaron.ballman wrote:

> It's a bit odd that this attribute has an AST node created for it but nothing is using that AST node elsewhere in the project. Are there other patches expected for making use of this attribute?


It gets used from Swift.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3330
+
+For example:
+
----------------
aaron.ballman wrote:
> I still feel like I'm lacking information about the domain. In the example, the domain is an uninitialized `const char *`, so how does this information get used by the attribute? Can I use any type of global I want? What about a literal?
I won't claim that I understand this well. Maybe @gribozavr2 or @milseman can explain this better. Literals are not allowed, however. The test explicitly forbids that.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9454
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;
----------------
aaron.ballman wrote:
> I don't think this requires a custom diagnostic -- we can use `err_attribute_argument_n_type` instead.
> I don't think this requires a custom diagnostic -- we can use `err_attribute_argument_n_type` instead.


Done. Can you please confirm that `n` is 1-based?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+    S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+        << identLoc->Ident;
+    return;
----------------
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005