[PATCH] D146342: [-Wunsafe-buffer-usage] Move the whole analysis to the end of a translation unit

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2340
+      if (Node->doesThisDeclarationHaveABody())
+        checkUnsafeBufferUsage(Node);
+      return true;
----------------
This code will grow bigger when more warnings are added, and it's quite likely that most warnings would want to duplicate themselves in each of the Visit methods. Maybe let's make a single function from which all per-function warnings are invoked?

Also, hmm, `UnsafeBufferUsageReporter` can probably be reused between callables.

Actually, here's an even fancier approach: maybe let's move the visitor outside the function so that it didn't get in the way of readability, and run warnings in a lambda so that we didn't need to explicitly capture anything:

```lang=c++
class CallableVisitor {
  llvm::function_ref<void(const Decl *)> callback;

public:
  CallableVisitor(llvm::function_ref<void(const Decl *)> callback):
    callback(callback)
  {}

  VisitFunctionDecl(FunctionDecl *Node) {
      if (Node->doesThisDeclarationHaveABody()) {
        callback(Node);
      }
  }

  // More Visit() methods.
};

void clang::sema::AnalysisBasedWarnings::IssueWarnings(const TranslationUnitDecl *TU) {
  // Common whole-TU safety checks go here.
  if (S.hasUncompilableErrorOccurred() || Diags.getIgnoreAllWarnings())
    return;

  // Whole-TU variables for all warnings go here. Then they're implicitly captured,
  // so no need to list them in the constructor anymore.
  UnsafeBufferUsageReporter UnsafeBufferUsageDiags(S);
  const bool UnsafeBufferUsageEmitFixits =
      Diags.getDiagnosticOptions().ShowFixits && S.getLangOpts().CPlusPlus20;
 
  CallableVisitor([&](const Decl *Node) {
    // Per-decl code for all warnings goes here.
    if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, D->getBeginLoc()) ||
        !Diags.isIgnored(diag::warn_unsafe_buffer_variable, D->getBeginLoc())) {
      checkUnsafeBufferUsage(Node, UnsafeBufferUsageDiags, UnsafeBufferUsageEmitFixits);
    }

  }).TraverseTranslationUnitDecl(TU);
}
```
I think that's a pretty neat way to organize this. This way it's immediately clear which code is executed how many times.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2369-2371
+  // Emit unsafe buffer usage warnings and fixits.
+  if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, SourceLocation()) ||
+      !Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation())) {
----------------
Ooo interesting, there is indeed a good reason to keep those checks outside; we shouldn't make an entire extra pass when none of the analysis-based warnings are enabled.

I suggest making the comment a bit more generic, so that users felt empowered to add more checks there and reuse the visitor for their warnings.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146342/new/

https://reviews.llvm.org/D146342