[PATCH] D84005: Introduce ns_error_domain attribute.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D84005#2171962 <https://reviews.llvm.org/D84005#2171962>, @MForster wrote:

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


Are there plans to use this attribute in Clang itself? If not, is there a way Swift can use attribute plugins instead of modifying Clang?



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3330
+
+For example:
+
----------------
MForster wrote:
> 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.
> Literals are not allowed, however. The test explicitly forbids that.

And I'm wondering why. For instance, attributes with enumeration arguments accept the enumerator as either an identifier or a string literal. I was sort of assuming that these domains are notionally like enumerators in that there is a list of domains, which is why I was asking about literals.


================
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">;
----------------
MForster wrote:
> 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?
Yes, the argument positions are 1-based.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5336
+  }
+  IdentifierLoc *identLoc =
+      Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
----------------
aaron.ballman wrote:
> Variables should match the usual coding style conventions.
It looks like `loc` was missed and still need to be updated to `Loc`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005