vsavchenko added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:2366
let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
let Documentation = [SuppressDocs];
}
----------------
aaron.ballman wrote:
> 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).
I decided not to change it yet because I was not sure that this is going to be
the final solution.
================
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;
----------------
aaron.ballman wrote:
> 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;
> ```
That's much better, thanks!
================
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;
----------------
aaron.ballman wrote:
> 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.)
This is actually something that I wanted to ask out of curiosity, how it works
as a declaration attribute if it's declared as `StmtAttr`
================
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);
----------------
aaron.ballman wrote:
>
Sure
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2885
+
+ if (It != Attrs.end()) {
+ return cast<SuppressAttr>(*It);
----------------
aaron.ballman wrote:
> The coding standard has us elide braces on single-line ifs (same comment
> applies elsewhere).
Sure!
================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2897
+ ASTContext &AC) {
+ PathDiagnosticLocation Location = BR.getLocation();
+
----------------
xazax.hun wrote:
> What will this location return? In case of a leak warning, we might get a
> different instance of the same warning on separate paths. We usually pick the
> shortest path, but it can change when we slightly alter the source code.
> Maybe we want the user to put the suppression at the uniqueing location when
> such location exist? (The allocation in case of a leak warnings.) I think
> that would result in a better user experience and more robust suppression
> mechanism. An open question is how to educate the user about the correct way
> of suppression. Should we emit a suppress location to the user explicitly?
Ah, leaks. I thought about those, and probably should've mentioned it here.
I think it is counter-intuitive to have separate locations for warnings
themselves and for suppressions. Because this would be the first place where
the user will try to put suppression. It is not obvious what to do with
locations when we report that something didn't happen when it should've. But
this group of checkers is relatively small, usually we do have a precise
location where something bad happens. So, I believe that leaks should be
addressed separately and not affect design for suppressions for the vast
majority of checkers.
Speaking of leaks, `RetainReleaseChecker` reports leaks on the allocation and
they can be suppressed that way.
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
