aaron.ballman added a comment.
In D93110#2448587 <https://reviews.llvm.org/D93110#2448587>, @vsavchenko wrote:
> Right now, I reused existing `suppress` attribute. However I did it in a
> rather unconventional manner. I allowed 0 arguments in one spelling and >1
> in another, which seems odd.
> I can see a couple other possible solutions here:
>
> - Choose a "keyword" that would be used to suppress all warnings (e.g.
> "suppress(all)")
> - Introduce two different suppress attributes (maybe even rename existing
> attribute to be GSLSuppress)
I don't think it's a problem that we use the same semantic attribute but with
different syntax requirements for the arguments. The semantics for GSL
suppression and Clang suppression are the same so it makes sense to me to use
the same semantic attribute so we don't wind up checking `isa<ThisAttr,
ThatAttr>(D)` everywhere. However, if it does start to get awkward, I think
splitting the attributes into two different semantic attributes is preferable
to using a keyword.
One usability concern I have with adding `[[clang::suppress]]` is that users
are going to want to use it to suppress all the various kinds of diagnostics
and not just clang static analyzer warnings. We get away with
`[[gsl::suppress]]` because that's very specific to the C++ core guidelines,
but we can't really hand-wave away the problem with `[[clang::suppress]]`. Have
you explored how this attribute will work with clang frontend diagnostics or
clang-tidy diagnostics? (This ignores some of the even harder diagnostics like
ones that percolate back up from the backend, but we should probably keep those
in mind as well.)
================
Comment at: clang/include/clang/Basic/Attr.td:2366
let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
let Documentation = [SuppressDocs];
}
----------------
The documentation will need to be updated for this. I have no idea if that may
be a bit awkward because we're documenting two different suppression mechanisms
(that do roughly the same thing).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4567
static void handleSuppressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
- if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+ if (AL.isCXX11Attribute() && !checkAttributeAtLeastNumArgs(S, AL, 1))
return;
----------------
The behavior here will be that `[[gsl::suppress]]` in C++11 mode and
`[[clang::suppress]]` in C++11 mode require at least one argument, while
`[[clang::supress]]` in C2x mode and `__attribute__((suppress))` in all
language modes allow any number of arguments (including zero). That doesn't
seem like what you want. I think you need something more like:
```
// The [[gsl::suppress]] spelling requires at least one argument, but the GNU
// and [[clang::suppress]] spellings do not require any arguments.
if (AL.hasScope() && AL.getScopeName()->isStr("gsl") &&
!checkAttributeAtLeastNumArgs(S, AL, 1))
return;
```
================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:56
SourceRange Range) {
- if (A.getNumArgs() < 1) {
+ if (A.isCXX11Attribute() && A.getNumArgs() < 1) {
S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments) << A << 1;
----------------
Same issue here.
(In other news, it looks like this attribute could stand to be updated to be a
`DeclOrStmtAttr` once D92800 lands -- that's not your problem to solve, more
just an observation.)
================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:69
// FIXME: Warn if the rule name is unknown. This is tricky because only
- // clang-tidy knows about available rules.
+ // clang-tidy and static analyzer know about available rules.
DiagnosticIdentifiers.push_back(RuleName);
----------------
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2885
+
+ if (It != Attrs.end()) {
+ return cast<SuppressAttr>(*It);
----------------
The coding standard has us elide braces on single-line ifs (same comment
applies elsewhere).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93110/new/
https://reviews.llvm.org/D93110
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
