aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:3816
+
+def Error : Attr {
+ let Spellings = [GCC<"error">];
----------------
I think this should be an inheritable attribute (same below) so that
redeclarations get the marking as well.
However, this does make for a bit of a novel situation with regards to other
attributes. The typical behavior for function attributes is:
```
void func(void);
void use1(void) {
func(); // Func is not marked yet, no attribute behavior
}
void func(void) __attribute__((whatever)));
void use2(void) {
func(); // Func is marked, attribute does something
}
void func(void) {} // Func is still marked because the attribute is inheritted
void use3(void) {
func(); // Func is marked, attribute does something
```
but because all of the interesting work is happening in the backend, I believe
the unmarked use will still act as though the attribute was marked.
================
Comment at: clang/include/clang/Basic/Attr.td:3819
+ let Args = [StringArgument<"UserDiagnostic">];
+ let Subjects = SubjectList<[Function], ErrorDiag>;
+ let Documentation = [ErrorAttrDocs];
----------------
ObjC methods as well?
================
Comment at: clang/include/clang/Basic/Attr.td:3823
+
+def Warning : Attr {
+ let Spellings = [GCC<"warning">];
----------------
Given that the only functional difference between these attributes is the
diagnostic level, I sort of wonder whether we should have one semantic
attribute with two spellings (one for warning, one for error) and an accessor
field to distinguish which is which.
Another interesting question is: are these attributes mutually exclusive? Can
they be duplicated (perhaps across declarations)? What happens if the messages
differ? (We may need some attribute merging logic to catch these situations.)
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6026
+ depend on optimizations, while providing diagnostics pointing to precise
+ locations of the call site in the source.
+ }];
----------------
I think the documentation for these should probably be combined into one
documentation blob; the only difference in behavior is whether the diagnostic
is a warning or an attribute.
I think the documentation needs to go into more details about how this
attribute works in practice. For example, what should I expect from code like:
```
struct Base {
__attribute__((warning("derp"))) virtual void foo();
};
struct Derived : Base {
void foo() override; // Does calling this also warn?
};
__attribute__((error("DERP!"))) void func() { // external function symbol!
func(); // Does this diagnose?
}
```
I suppose another interesting question given the design is to use the optimizer
is: what about LTO? Say I have:
```
// TU1.c
__attribute__((error("Derp Derp Derp"))) void func(void);
// TU.c
extern void func(void);
void blerp(void) { func(); }
```
What should happen and does LTO change the answer?
================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:75
+def err_fe_backend_user_diagnostic :
+ Error<"call to %0 declared with attribute error: %1">, BackendInfo;
----------------
Should probably give these names that relate to the attribute. How about
`err_fe_backend_error_attr` (and `warn_fe_backend_warn_attr`) or something
along those lines?
================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:78
+def warn_fe_backend_user_diagnostic :
+ Warning<"call to %0 declared with attribute warning: %1">, BackendInfo,
InGroup<BackendUserDiagnostic>;
+
----------------
80-col limit
================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
----------------
GCC doesn't seem to support this spelling -- do they have a different one we
should reuse?
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5308-5310
+ if (TargetDecl)
+ if (TargetDecl->hasAttr<ErrorAttr>() ||
+ TargetDecl->hasAttr<WarningAttr>()) {
----------------
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:776
+
+ CalleeName = FD->getName();
+ }
----------------
This isn't really safe -- not all functions have names that can be retrieved
this way. We should be sure to add some test coverage for functions without
"names" (like `operator int()`).
I think it's better to call `getNameForDiagnostic()` here (unless the
diagnostic printer does that already for a `DeclarationName`, in which case
`getDeclName()` would be better.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+ else
+ Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
I think the usability in this situation is a concern -- only having a function
name but no source location information is pretty tricky. Do you have a sense
for how often we would hit this case?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:949-960
+static void handleErrorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+ StringRef Str;
+ if (!S.checkStringLiteralArgumentAttr(AL, 0, Str))
+ return;
+ D->addAttr(::new (S.Context) ErrorAttr(S.Context, AL, Str));
+}
+static void handleWarningAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
----------------
Pretty sure you can get rid of the manual handlers and just use `SimpleHandler
= 1` in Attr.td, unless we need to add extra diagnostic logic (which we might
need to do for attribute merging).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106030/new/
https://reviews.llvm.org/D106030
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
