[analyzer] Add forwarding `addVisitor` method

[vsavchenko]

Summary

The majority of all addVisitor callers follow the same pattern:

addVisitor(std::make_unique<SomeVisitor>(arg1, arg2, ...));

This patches introduces additional overload for addVisitor to simplify
that pattern:

addVisitor<SomeVisitor>(arg1, arg2, ...);

[NoQ]

Excellent.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

public:
   /// Reports are uniqued to ensure that we do not emit multiple diagnostics
   /// for each bug.
   void Profile(llvm::FoldingSetNodeID &hash) const override;
 
   /// Add custom or predefined bug report visitors to this report.
   ///
   /// The visitors should be used when the default trace is not sufficient.
   /// For example, they allow constructing a more elaborate trace.
-  /// \sa registerConditionVisitor(), registerTrackNullOrUndefValue(),
-  /// registerFindLastStore(), registerNilReceiverVisitor(), and
-  /// registerVarDeclsLastStore().
+  /// @{
   void addVisitor(std::unique_ptr<BugReporterVisitor> visitor);
 
+  template <class VisitorType, class... Args>
+  void addVisitor(Args &&... ConstructorArgs) {
+    addVisitor(
+        std::make_unique<VisitorType>(std::forward<Args>(ConstructorArgs)...));
+  }
+  /// @}
+
   /// Remove all visitors attached to this bug report.
   void clearVisitors();
 
   /// Iterators through the custom diagnostic visitors.
   visitor_iterator visitor_begin() { return Callbacks.begin(); }
   visitor_iterator visitor_end() { return Callbacks.end(); }
   visitor_range visitors() { return {visitor_begin(), visitor_end()}; }
 

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (OwnershipTransferred) {
         if (printMemFnName(DeallocOs, C, DeallocExpr))
           os << ", not " << DeallocOs.str();
     }
 
     auto R = std::make_unique<PathSensitiveBugReport>(*BT_MismatchedDealloc,
                                                       os.str(), N);
     R->markInteresting(Sym);
     R->addRange(Range);
-    R->addVisitor(std::make_unique<MallocBugVisitor>(Sym));
+    R->addVisitor<MallocBugVisitor>(Sym);
     C.emitReport(std::move(R));
   }
 }
 
 void MallocChecker::HandleOffsetFree(CheckerContext &C, SVal ArgVal,
                                      SourceRange Range, const Expr *DeallocExpr,
                                      AllocationFamily Family,
                                      const Expr *AllocExpr) const {
auto R = std::make_unique<PathSensitiveBugReport>(
         *BT_UseFree[*CheckKind],
         AF == AF_InnerBuffer
             ? "Inner pointer of container used after re/deallocation"
             : "Use of memory after it is freed",
         N);
 
     R->markInteresting(Sym);
     R->addRange(Range);
-    R->addVisitor(std::make_unique<MallocBugVisitor>(Sym));
+    R->addVisitor<MallocBugVisitor>(Sym);
 
     if (AF == AF_InnerBuffer)
       R->addVisitor(allocation_state::getInnerPointerBRVisitor(Sym));
 
     C.emitReport(std::move(R));
   }
 }
 
auto R = std::make_unique<PathSensitiveBugReport>(
         *BT_DoubleFree[*CheckKind],
         (Released ? "Attempt to free released memory"
                   : "Attempt to free non-owned memory"),
         N);
     R->addRange(Range);
     R->markInteresting(Sym);
     if (PrevSym)
       R->markInteresting(PrevSym);
-    R->addVisitor(std::make_unique<MallocBugVisitor>(Sym));
+    R->addVisitor<MallocBugVisitor>(Sym);
     C.emitReport(std::move(R));
   }
 }
 
 void MallocChecker::HandleDoubleDelete(CheckerContext &C, SymbolRef Sym) const {
 
   if (!ChecksEnabled[CK_NewDeleteChecker]) {
     C.addSink();
if (!BT_DoubleDelete)
       BT_DoubleDelete.reset(new BugType(CheckNames[CK_NewDeleteChecker],
                                         "Double delete",
                                         categories::MemoryError));
 
     auto R = std::make_unique<PathSensitiveBugReport>(
         *BT_DoubleDelete, "Attempt to delete released memory", N);
 
     R->markInteresting(Sym);
-    R->addVisitor(std::make_unique<MallocBugVisitor>(Sym));
+    R->addVisitor<MallocBugVisitor>(Sym);
     C.emitReport(std::move(R));
   }
 }
 
 void MallocChecker::HandleUseZeroAlloc(CheckerContext &C, SourceRange Range,
                                        SymbolRef Sym) const {
 
   if (!ChecksEnabled[CK_MallocChecker] && !ChecksEnabled[CK_NewDeleteChecker]) {
if (!BT_UseZerroAllocated[*CheckKind])
                       categories::MemoryError));
 
     auto R = std::make_unique<PathSensitiveBugReport>(
         *BT_UseZerroAllocated[*CheckKind], "Use of zero-allocated memory", N);
 
     R->addRange(Range);
     if (Sym) {
       R->markInteresting(Sym);
-      R->addVisitor(std::make_unique<MallocBugVisitor>(Sym));
+      R->addVisitor<MallocBugVisitor>(Sym);
     }
     C.emitReport(std::move(R));
   }
 }
 
 void MallocChecker::HandleFunctionPtrFree(CheckerContext &C, SVal ArgVal,
                                           SourceRange Range,
                                           const Expr *FreeExpr,
void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
   } else {
     os << "Potential memory leak";
   }
 
   auto R = std::make_unique<PathSensitiveBugReport>(
       *BT_Leak[*CheckKind], os.str(), N, LocUsedForUniqueing,
       AllocNode->getLocationContext()->getDecl());
   R->markInteresting(Sym);
-  R->addVisitor(std::make_unique<MallocBugVisitor>(Sym, true));
+  R->addVisitor<MallocBugVisitor>(Sym, true);
   C.emitReport(std::move(R));
 }
 
 void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                      CheckerContext &C) const
 {
   ProgramStateRef state = C.getState();
   RegionStateTy OldRS = state->get<RegionState>();

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

private:
 
   void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
                  const MemRegion *Region, BugReporter &BR,
                  const Stmt *ValueExpr = nullptr) const {
     const std::unique_ptr<BugType> &BT = getBugType(CK);
     auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
     if (Region) {
       R->markInteresting(Region);
-      R->addVisitor(std::make_unique<NullabilityBugVisitor>(Region));
+      R->addVisitor<NullabilityBugVisitor>(Region);
     }
     if (ValueExpr) {
       R->addRange(ValueExpr->getSourceRange());
       if (Error == ErrorKind::NilAssignedToNonnull ||
           Error == ErrorKind::NilPassedToNonnull ||
           Error == ErrorKind::NilReturnedToNonnull)
         if (const auto *Ex = dyn_cast<Expr>(ValueExpr))
           bugreporter::trackExpressionValue(N, Ex, *R);

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

RefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
 RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
                                ExplodedNode *n, SymbolRef sym, bool isLeak)
     : PathSensitiveBugReport(D, D.getDescription(), n), Sym(sym),
       isLeak(isLeak) {
   if (!isLeak)
-    addVisitor(std::make_unique<RefCountReportVisitor>(sym));
+    addVisitor<RefCountReportVisitor>(sym);
 }
 
 RefCountReport::RefCountReport(const RefCountBug &D, const LangOptions &LOpts,
                                ExplodedNode *n, SymbolRef sym,
                                StringRef endText)
     : PathSensitiveBugReport(D, D.getDescription(), endText, n) {
 
-  addVisitor(std::make_unique<RefCountReportVisitor>(sym));
+  addVisitor<RefCountReportVisitor>(sym);
 }
 
 void RefLeakReport::deriveParamLocation(CheckerContext &Ctx) {
   const SourceManager &SMgr = Ctx.getSourceManager();
 
   if (!Sym->getOriginRegion())
     return;
 
if (!AllVarBindings.empty() &&
     // 'AllocFirstBinding', we need to explain how the leaking object
     // got from one to another.
     //
     // NOTE: We use the actual SVal stored in AllocBindingToReport here because
     //       FindLastStoreBRVisitor compares SVal's and it can get trickier for
     //       something like derived regions if we want to construct SVal from
     //       Sym. Instead, we take the value that is definitely stored in that
     //       region, thus guaranteeing that FindLastStoreBRVisitor will work.
-    addVisitor(std::make_unique<FindLastStoreBRVisitor>(
+    addVisitor<FindLastStoreBRVisitor>(
         AllVarBindings[0].second.castAs<KnownSVal>(), AllocBindingToReport,
-        false, bugreporter::TrackingKind::Thorough));
+        false, bugreporter::TrackingKind::Thorough);
   } else {
     AllocBindingToReport = AllocFirstBinding;
   }
 }
 
 RefLeakReport::RefLeakReport(const RefCountBug &D, const LangOptions &LOpts,
                              ExplodedNode *N, SymbolRef Sym,
                              CheckerContext &Ctx)
     : RefCountReport(D, LOpts, N, Sym, /*isLeak=*/true) {
 
   deriveAllocLocation(Ctx);
   findBindingToReport(Ctx, N);
 
   if (!AllocFirstBinding)
     deriveParamLocation(Ctx);
 
   createDescription(Ctx);
 
-  addVisitor(std::make_unique<RefLeakReportVisitor>(Sym, AllocBindingToReport));
+  addVisitor<RefLeakReportVisitor>(Sym, AllocBindingToReport);
 }

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

while (BugPathInfo *BugPath = BugGraph.getNextBugPath()) {
     // Find the BugReport with the original location.
     PathSensitiveBugReport *R = BugPath->Report;
     assert(R && "No original report found for sliced graph.");
     assert(R->isValid() && "Report selected by trimmed graph marked invalid.");
     const ExplodedNode *ErrorNode = BugPath->ErrorNode;
 
     // Register refutation visitors first, if they mark the bug invalid no
     // further analysis is required
-    R->addVisitor(std::make_unique<LikelyFalsePositiveSuppressionBRVisitor>());
+    R->addVisitor<LikelyFalsePositiveSuppressionBRVisitor>();
 
     // Register additional node visitors.
-    R->addVisitor(std::make_unique<NilReceiverBRVisitor>());
-    R->addVisitor(std::make_unique<ConditionBRVisitor>());
-    R->addVisitor(std::make_unique<TagVisitor>());
+    R->addVisitor<NilReceiverBRVisitor>();
+    R->addVisitor<ConditionBRVisitor>();
+    R->addVisitor<TagVisitor>();
 
     BugReporterContext BRC(Reporter);
 
     // Run all visitors on a given graph, once.
     std::unique_ptr<VisitorsDiagnosticsTy> visitorNotes =
         generateVisitorsDiagnostics(R, ErrorNode, BRC);
 
     if (R->isValid()) {
       if (Reporter.getAnalyzerOptions().ShouldCrosscheckWithZ3) {
         // If crosscheck is enabled, remove all visitors, add the refutation
         // visitor and check again
         R->clearVisitors();
-        R->addVisitor(std::make_unique<FalsePositiveRefutationBRVisitor>());
+        R->addVisitor<FalsePositiveRefutationBRVisitor>();
 
         // We don't overwrite the notes inserted by other visitors because the
         // refutation manager does not add any new note to the path
         generateVisitorsDiagnostics(R, BugPath->ErrorNode, BRC);
       }
 
       // Check if the bug is still valid
       if (R->isValid())

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
     return nullptr;
   }
 
   static void addMacroVisitorIfNecessary(
         const ExplodedNode *N, const MemRegion *R,
         bool EnableNullFPSuppression, PathSensitiveBugReport &BR,
         const SVal V) {
     AnalyzerOptions &Options = N->getState()->getAnalysisManager().options;
-    if (EnableNullFPSuppression &&
-        Options.ShouldSuppressNullReturnPaths && V.getAs<Loc>())
-      BR.addVisitor(std::make_unique<MacroNullReturnSuppressionVisitor>(
-              R->getAs<SubRegion>(), V));
+    if (EnableNullFPSuppression && Options.ShouldSuppressNullReturnPaths &&
+        V.getAs<Loc>())
+      BR.addVisitor<MacroNullReturnSuppressionVisitor>(R->getAs<SubRegion>(),
+                                                       V);
   }
 
   void* getTag() const {
     static int Tag = 0;
     return static_cast<void *>(&Tag);
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
     if (cast<Expr>(S)->isGLValue())
       if (Optional<Loc> LValue = RetVal.getAs<Loc>())
         RetVal = State->getSVal(*LValue);
 
     // See if the return value is NULL. If so, suppress the report.
     AnalyzerOptions &Options = State->getAnalysisManager().options;
 
     bool EnableNullFPSuppression = false;
-    if (InEnableNullFPSuppression &&
-        Options.ShouldSuppressNullReturnPaths)
+    if (InEnableNullFPSuppression && Options.ShouldSuppressNullReturnPaths)
       if (Optional<Loc> RetLoc = RetVal.getAs<Loc>())
         EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
-    BR.addVisitor(std::make_unique<ReturnVisitor>(CalleeContext,
-                                                   EnableNullFPSuppression,
-                                                   Options, TKind));
+    BR.addVisitor<ReturnVisitor>(CalleeContext, EnableNullFPSuppression,
+                                 Options, TKind);
   }
 
   PathDiagnosticPieceRef visitNodeInitial(const ExplodedNode *N,
                                           BugReporterContext &BRC,
                                           PathSensitiveBugReport &BR) {
     // Only print a message at the interesting return statement.
     if (N->getLocationContext() != CalleeSFC)
       return nullptr;
if (DS) {
       if (VR) {
         // See if we can get the BlockVarRegion.
         ProgramStateRef State = StoreSite->getState();
         SVal V = StoreSite->getSVal(S);
         if (const auto *BDR =
               dyn_cast_or_null<BlockDataRegion>(V.getAsRegion())) {
           if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
             if (auto KV = State->getSVal(OriginalR).getAs<KnownSVal>())
-              BR.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
-                  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC));
+              BR.addVisitor<FindLastStoreBRVisitor>(
+                  *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC);
           }
         }
       }
     }
     if (action)
       showBRDiagnostics(action, os, R, V, OldRegion, DS);
 
   } else if (StoreSite->getLocation().getAs<CallEnter>()) {
bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
   const StackFrameContext *SFC = LVNode->getStackFrame();
 
   // We only track expressions if we believe that they are important. Chances
   // are good that control dependencies to the tracking point are also important
   // because of this, let's explain why we believe control reached this point.
   // TODO: Shouldn't we track control dependencies of every bug location, rather
   // than only tracked expressions?
   if (LVState->getAnalysisManager().getAnalyzerOptions().ShouldTrackConditions)
-    report.addVisitor(std::make_unique<TrackControlDependencyCondBRVisitor>(
-          InputNode));
+    report.addVisitor<TrackControlDependencyCondBRVisitor>(InputNode);
 
   // The message send could be nil due to the receiver being nil.
   // At this point in the path, the receiver should be live since we are at the
   // message send expr. If it is nil, start tracking it.
   if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, LVNode))
     trackExpressionValue(
         LVNode, Receiver, report, TKind, EnableNullFPSuppression);
 
if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
     const MemRegion *RR = getLocationRegionIfReference(Inner, LVNode);
     bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue();
 
     // If this is a C++ reference to a null pointer, we are tracking the
     // pointer. In addition, we should find the store at which the reference
     // got initialized.
     if (RR && !LVIsNull)
       if (auto KV = LVal.getAs<KnownSVal>())
-        report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
-            *KV, RR, EnableNullFPSuppression, TKind, SFC));
+        report.addVisitor<FindLastStoreBRVisitor>(
+            *KV, RR, EnableNullFPSuppression, TKind, SFC);
 
     // In case of C++ references, we want to differentiate between a null
     // reference and reference to null pointer.
     // If the LVal is null, check if we are dealing with null reference.
     // For those, we want to track the location of the reference.
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
     if (R) {
 
       // Mark both the variable region and its contents as interesting.
       SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
-      report.addVisitor(
-          std::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R), TKind));
+      report.addVisitor<NoStoreFuncVisitor>(cast<SubRegion>(R), TKind);
 
       MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
           LVNode, R, EnableNullFPSuppression, report, V);
 
       report.markInteresting(V, TKind);
-      report.addVisitor(std::make_unique<UndefOrNullArgVisitor>(R));
+      report.addVisitor<UndefOrNullArgVisitor>(R);
 
       // If the contents are symbolic and null, find out when they became null.
       if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true))
         if (LVState->isNull(V).isConstrainedTrue())
-          report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
-              V.castAs<DefinedSVal>(), false));
+          report.addVisitor<TrackConstraintBRVisitor>(V.castAs<DefinedSVal>(),
+                                                      false);
 
       // Add visitor, which will suppress inline defensive checks.
       if (auto DV = V.getAs<DefinedSVal>())
         if (!DV->isZeroConstant() && EnableNullFPSuppression) {
           // Note that LVNode may be too late (i.e., too far from the InputNode)
           // because the lvalue may have been computed before the inlined call
           // was evaluated. InputNode may as well be too early here, because
           // the symbol is already dead; this, however, is fine because we can
           // still find the node in which it collapsed to null previously.
-          report.addVisitor(
-              std::make_unique<SuppressInlineDefensiveChecksVisitor>(
-                  *DV, InputNode));
+          report.addVisitor<SuppressInlineDefensiveChecksVisitor>(*DV,
+                                                                  InputNode);
         }
 
       if (auto KV = V.getAs<KnownSVal>())
-        report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
-            *KV, R, EnableNullFPSuppression, TKind, SFC));
+        report.addVisitor<FindLastStoreBRVisitor>(
+            *KV, R, EnableNullFPSuppression, TKind, SFC);
       return true;
     }
   }
 
   // If the expression is not an "lvalue expression", we can still
   // track the constraints on its contents.
   SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
 
if (auto L = V.getAs<loc::MemRegionVal>()) {
     // Try to use the correct type when looking up the value.
     SVal RVal;
     if (ExplodedGraph::isInterestingLValueExpr(Inner))
       RVal = LVState->getRawSVal(L.getValue(), Inner->getType());
     else if (CanDereference)
       RVal = LVState->getSVal(L->getRegion());
 
     if (CanDereference) {
-      report.addVisitor(
-          std::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
+      report.addVisitor<UndefOrNullArgVisitor>(L->getRegion());
 
       if (auto KV = RVal.getAs<KnownSVal>())
-        report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
-            *KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC));
+        report.addVisitor<FindLastStoreBRVisitor>(
+            *KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC);
     }
 
     const MemRegion *RegionRVal = RVal.getAsRegion();
-    if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
+    if (isa_and_nonnull<SymbolicRegion>(RegionRVal)) {
       report.markInteresting(RegionRVal, TKind);
-      report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
-            loc::MemRegionVal(RegionRVal), /*assumption=*/false));
+      report.addVisitor<TrackConstraintBRVisitor>(loc::MemRegionVal(RegionRVal),
+                                                  /*assumption=*/false);
     }
   }
 
   if (Inner->isRValue())
     trackRValueExpression(LVNode, Inner, report, TKind,
                           EnableNullFPSuppression);
 
   return true;