[PATCH] D84005: Introduce ns_error_domain attribute.

[Michael Forster via Phabricator via cfe-commits]

MForster added a comment.

I have updated the attribute documentation to include the additional information provided by Doug. I think adding additional diagnostics would rather be separate changes.

I think I have addressed all remaining review comments.



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5344
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+      !lookupResult.getAsSingle<VarDecl>()) {
+    S.Diag(IdentLoc->Loc, diag::err_nserrordomain_invalid_decl)
----------------
riccibruno wrote:
> Just a note that `LookupResult::getAsSingle` has tricky semantics (returns null if the result is not exactly `LookupResultKind::Found`) and has been (and still is) the source of many bugs in clang.
> 
> (Example: my favourite one is still the silly:
> ```
> struct S {
>   void not_overloaded();
>   enum { not_overloaded }; // error; redefinition of 'not_overloaded'
> 
>   void overloaded();
>   void overloaded(int);
>   enum { overloaded }; // clang is fine with this!
> };
> ```
> )
> 
> I don't know if it is a problem here or not though.
> Just a note that LookupResult::getAsSingle has tricky semantics [...]

Thanks for the advice, but I think this is not an issue here. The check should only succeed if there is a unique lookup result.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84005