aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9450
+def err_nserrordomain_not_tagdecl : Error<
+ "ns_error_domain attribute only valid on "
+ "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;
----------------
ns_error_domain -> 'ns_error_domain' (with the single quotes around it, because
it's syntax).
================
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">;
----------------
I don't think this requires a custom diagnostic -- we can use
`err_attribute_argument_n_type` instead.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5332
if (!isa<TagDecl>(D)) {
- S.Diag(D->getLocStart(), diag::err_nserrordomain_not_tagdecl)
+ S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
<< S.getLangOpts().CPlusPlus;
----------------
aaron.ballman wrote:
> Where is this error defined?
> Where is this error defined?
Now I found it, it was dropped in the version of the review I was looking at.
:-)
================
Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+
----------------
I think this test belongs in Sema, not in Analysis (nothing about the feature
is specific to static analysis).
Also, missing Sema tests that the attribute appertains to the correct subject,
accepts the correct number and types of arguments, etc.
I'd also appreciate seeing tests of the edge cases, like a locally-defined
enumeration, a forward declaration of an enumeration, etc.
================
Comment at: clang/test/Analysis/ns_error_enum.m:1
+// RUN: %clang_cc1 -verify %s
+
----------------
MForster wrote:
> gribozavr2 wrote:
> > This file is a `.m` -- any specific reason? I'd call it `.c` and run the
> > test in C, Objective-C, and C++ modes (enums might work slightly
> > differently, the name lookup functionality might work differently).
> The test doesn't compile in C or C++ (`non-defining declaration of
> enumeration with a fixed underlying type is only permitted as a standalone
> declaration; missing list of enumerators?`). Not sure if it's worth adapting.
Enums with fixed underlying types exist in C++ and C, so I was expecting the
attribute to work there. If the attribute isn't supported in these languages,
should the attribute be tied to a language mode?
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
