aaron.ballman added a comment.
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?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:3330
+
+For example:
+
----------------
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?
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9456
" attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+ "ns_error_domain attribute only valid on "
----------------
This diagnostic doesn't match the attribute definition (the subject list only
lists enumerations). You should be able to remove this diagnostic entirely and
just rely on the common attribute handler to diagnose incorrect subjects,
though you should add an `ErrorDiag` to the subject list in Attr.td.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5330
+static void handleNSErrorDomain(Sema &S, Decl *D, const ParsedAttr &Attr) {
+ if (!isa<TagDecl>(D)) {
----------------
Please don't use `Attr` as a declaration identifier, it's the name of a type
already and that confuses some editors.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5331
+static void handleNSErrorDomain(Sema &S, Decl *D, const ParsedAttr &Attr) {
+ if (!isa<TagDecl>(D)) {
+ S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
----------------
This code should be removed, the common attribute handler already diagnoses
incorrect subjects (and this doesn't match the interface in Attr.td anyway).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5336
+ }
+ IdentifierLoc *identLoc =
+ Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
----------------
Variables should match the usual coding style conventions.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5355
+ S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+ << identLoc->Ident;
+ return;
----------------
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?
================
Comment at: clang/test/Analysis/ns_error_enum.c:1
+// RUN: %clang_cc1 -verify %s -x c -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
----------------
The test should still be moved out of Analysis and into Sema.
================
Comment at: clang/test/Analysis/ns_error_enum.c:3
+// RUN: %clang_cc1 -verify %s -x c++ -Wno-elaborated-enum-base
+// RUN: %clang_cc1 -verify %s -x objective-c
+
----------------
One more run line for `objective-c++`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84005/new/
https://reviews.llvm.org/D84005
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
