[PATCH] D93110: [analyzer] Implement a first version of suppressions via attributes

[Valeriy Savchenko via Phabricator via cfe-commits]

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