[analyzer][NFC] NoStoreFuncVisitor: Compact parameters for region pretty printing into a class

[Szelethus]

Summary

Exactly what it says on the tin! I personally find this easier to read.

[vsavchenko]

Honestly, I don't really see how this is better.
IMO Printer is something that prints, it should be everything that it does. It can accept different parameters tweaking how it prints in its constructor, but if it is a region printer, you should give it a region, and it should print it. It's not a one-use thing.

Here it looks like you aggregate all the information about the region inside of this class, and it knows how to print it. And it looks like this is actually a primary reason here, you want a good way to aggregate all these million parameters into one single object. A better name (and purpose) for abstraction like this would be to actually make all these parameters to make sense together. So that the visitor produces them together and then we process them together. And one nice property of such aggregated result would be ability to print it.

Another problem that I see is that even for such a small class, I have no idea how to use it. It has too many parameters and it doesn't tell me what is the relationship between them. My guess is that in this form this class will never be reused outside of NoStoreFuncVisitor, then why do we need it?

At this point, I don't see how this abstraction adds any meaningful layering. Meaningful abstraction defines a language for a problem that we are trying to solve, so we can use it naturally when we talk.

Sorry about being pretty harsh here. I do think that NoStoreFuncVisitor needs refactoring and better abstractions, I just don't think that this is the way to go.

[Szelethus]

In D105552#2861924, @vsavchenko wrote:

Sorry about being pretty harsh here. I do think that NoStoreFuncVisitor needs refactoring and better abstractions, I just don't think that this is the way to go.

Nah, you're totally right. In retrospect, this refactoring feela a lot like it was done by a robot, not a human. It was a part of some greater plan that I abandoned later, so its luckily not a consequential change to my overall refactoring efforts.

Cheers for the enlightening! I suppose I never saw similar changed that way.

[Szelethus]

Actually, one thing might be worth considering. The grand idea is to split up NoStoreFuncVisitor so that its clients only need to define these 3 (quoting from D105553):

• What constitutes as a change to an entity
• What the diagnostic message should be
• What constitutes as the entity of interest being passed into the function

In the end, in my eye, it would look like this:

                     <--- NoStateChangeToRegionVisitor <--- NoStoreFuncVisitor
                    /
NoStateChangeVisitor                                   <--- NoDynamicMemoryOwnershipChangeVisitor
                    \                                 /
                     <--- NoStateChangeToSymbolVisitor
                                                      \
                                                       <--- NoStreamOwnershipChangeVisitor

• NoStateChangeVisitor will ask its clients the above mentioned 3 questions.
• NoStateChangeToSymbolVisitor and NoStateChangeToRegionVisitor would answer whether the entity of interest was actually passed into the function, and propagate the rest.
• Visitors on the bottom would take care of what remains.

With this, or a similar change, visitors handling state changes to regions would need to know even less about the job that NoStateChangeToRegionVisitor supposedly took over already.

With that said, I don't have another NoStateChangeToRegionVisitor in my mind for the short term future, and even if I did, such a cosmetic encapsulation could wait for a better thought out solution.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 /// Put a diagnostic on return statement of all inlined functions
 /// for which  the region of interest \p RegionOfInterest was passed into,
 /// but not written inside, and it has caused an undefined read or a null
 /// pointer dereference outside.
 class NoStoreFuncVisitor final : public BugReporterVisitor {
   const SubRegion *RegionOfInterest;
   MemRegionManager &MmrMgr;
   const SourceManager &SM;
-  const PrintingPolicy &PP;
   bugreporter::TrackingKind TKind;
 
   /// Recursion limit for dereferencing fields when looking for the
   /// region of interest.
   /// The limit of two indicates that we will dereference fields only once.
   static const unsigned DEREFERENCE_LIMIT = 2;
 
   /// Frames writing into \c RegionOfInterest.
   /// This visitor generates a note only if a function does not write into
   /// a region of interest. This information is not immediately available
   /// by looking at the node associated with the exit from the function
   /// (usually the return statement). To avoid recomputing the same information
   /// many times (going up the path for each node and checking whether the
   /// region was written into) we instead lazily compute the
   /// stack frames along the path which write into the region of interest.
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingRegion;
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
 
   using RegionVector = SmallVector<const MemRegion *, 5>;
 
+  class RegionPrinter {
+    const RegionVector &FieldChain;
+    const MemRegion *RegionOfInterest;
+    const MemRegion *MatchedRegion;
+    const StringRef FirstElement;
+    const bool FirstIsReferenceType;
+    unsigned IndirectionLevel;
+    const PrintingPolicy &PP;
+
+  private:
+    /// Print first item in the chain, return new separator.
+    static StringRef prettyPrintFirstElement(StringRef FirstElement,
+                                             bool MoreItemsExpected,
+                                             int IndirectionLevel,
+                                             llvm::raw_svector_ostream &os);
+
+  public:
+    RegionPrinter(const RegionVector &FieldChain,
+                  const MemRegion *RegionOfInterest,
+                  const MemRegion *MatchedRegion, StringRef FirstElement,
+                  bool FirstIsReferenceType, unsigned IndirectionLevel)
+        : FieldChain(FieldChain), RegionOfInterest(RegionOfInterest),
+          MatchedRegion(MatchedRegion), FirstElement(FirstElement),
+          FirstIsReferenceType(FirstIsReferenceType),
+          IndirectionLevel(IndirectionLevel),
+          PP(RegionOfInterest->getMemRegionManager()
+                 .getContext()
+                 .getPrintingPolicy()) {}
+
+    /// Pretty-print region \p MatchedRegion to \p os.
+    /// \return Whether printing succeeded.
+    bool prettyPrintRegionName(llvm::raw_svector_ostream &os);
+  };
+
 public:
   NoStoreFuncVisitor(const SubRegion *R, bugreporter::TrackingKind TKind)
       : RegionOfInterest(R), MmrMgr(R->getMemRegionManager()),
-        SM(MmrMgr.getContext().getSourceManager()),
-        PP(MmrMgr.getContext().getPrintingPolicy()), TKind(TKind) {}
+        SM(MmrMgr.getContext().getSourceManager()), TKind(TKind) {}
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     static int Tag = 0;
     ID.AddPointer(&Tag);
     ID.AddPointer(RegionOfInterest);
   }
 
   void *getTag() const {
private:
   /// Write to \c FramesModifyingRegion all stack frames along
   /// the path in the current stack frame which modify \c RegionOfInterest.
   void findModifyingFrames(const ExplodedNode *N);
 
   /// Consume the information on the no-store stack frame in order to
   /// either emit a note or suppress the report enirely.
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
-  PathDiagnosticPieceRef
-  maybeEmitNote(PathSensitiveBugReport &R, const CallEvent &Call,
-                const ExplodedNode *N, const RegionVector &FieldChain,
-                const MemRegion *MatchedRegion, StringRef FirstElement,
-                bool FirstIsReferenceType, unsigned IndirectionLevel);
-
-  /// Pretty-print region \p MatchedRegion to \p os.
-  /// \return Whether printing succeeded.
-  bool prettyPrintRegionName(StringRef FirstElement, bool FirstIsReferenceType,
-                             const MemRegion *MatchedRegion,
-                             const RegionVector &FieldChain,
-                             int IndirectionLevel,
-                             llvm::raw_svector_ostream &os);
-
-  /// Print first item in the chain, return new separator.
-  static StringRef prettyPrintFirstElement(StringRef FirstElement,
-                                           bool MoreItemsExpected,
-                                           int IndirectionLevel,
-                                           llvm::raw_svector_ostream &os);
+  PathDiagnosticPieceRef maybeEmitNote(PathSensitiveBugReport &R,
+                                       const CallEvent &Call,
+                                       const ExplodedNode *N, RegionPrinter RP);
 };
 
 } // end of anonymous namespace
 
 /// \return Whether the method declaration \p Parent
 /// syntactically has a binary operation writing into the ivar \p Ivar.
 static bool potentiallyWritesIntoIvar(const Decl *Parent,
                                       const ObjCIvarDecl *Ivar) {
NoStoreFuncVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BR,
   // Region of interest corresponds to an IVar, exiting a method
   // which could have written into that IVar, but did not.
   if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {
     if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest)) {
       const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
       if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
           potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
                                     IvarR->getDecl()))
-        return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
-                             /*FirstIsReferenceType=*/false, 1);
+        return maybeEmitNote(R, *Call, N,
+                             RegionPrinter{{},
+                                           RegionOfInterest,
+                                           SelfRegion,
+                                           "self",
+                                           /*FirstIsReferenceType=*/false,
+                                           1});
     }
   }
 
   if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
     const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
     if (RegionOfInterest->isSubRegionOf(ThisR) &&
         !CCall->getDecl()->isImplicit())
-      return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
-                           /*FirstIsReferenceType=*/false, 1);
+      return maybeEmitNote(R, *Call, N,
+                           RegionPrinter{{},
+                                         RegionOfInterest,
+                                         ThisR,
+                                         "this",
+                                         /*FirstIsReferenceType=*/false,
+                                         1});
 
     // Do not generate diagnostics for not modified parameters in
     // constructors.
     return nullptr;
   }
 
   ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
   for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
     const ParmVarDecl *PVD = parameters[I];
     SVal V = Call->getArgSVal(I);
     bool ParamIsReferenceType = PVD->getType()->isReferenceType();
     std::string ParamName = PVD->getNameAsString();
 
-    int IndirectionLevel = 1;
+    unsigned IndirectionLevel = 1;
     QualType T = PVD->getType();
     while (const MemRegion *MR = V.getAsRegion()) {
       if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-        return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
-                             ParamIsReferenceType, IndirectionLevel);
+        return maybeEmitNote(R, *Call, N,
+                             RegionPrinter{{},
+                                           RegionOfInterest,
+                                           MR,
+                                           ParamName,
+                                           ParamIsReferenceType,
+                                           IndirectionLevel});
 
       QualType PT = T->getPointeeType();
       if (PT.isNull() || PT->isVoidType())
         break;
 
       if (const RecordDecl *RD = PT->getAsRecordDecl())
         if (Optional<RegionVector> P =
                 findRegionOfInterestInRecord(RD, State, MR))
-          return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
-                               ParamIsReferenceType, IndirectionLevel);
+          return maybeEmitNote(
+              R, *Call, N,
+              RegionPrinter{*P, RegionOfInterest, RegionOfInterest, ParamName,
+                            ParamIsReferenceType, IndirectionLevel});
 
       V = State->getSVal(MR, PT);
       T = PT;
       IndirectionLevel++;
     }
   }
 
   return nullptr;
do {
 
     N = N->getFirstPred();
   } while (N);
 }
 
 static llvm::StringLiteral WillBeUsedForACondition =
     ", which participates in a condition later";
 
-PathDiagnosticPieceRef NoStoreFuncVisitor::maybeEmitNote(
-    PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N,
-    const RegionVector &FieldChain, const MemRegion *MatchedRegion,
-    StringRef FirstElement, bool FirstIsReferenceType,
-    unsigned IndirectionLevel) {
+PathDiagnosticPieceRef
+NoStoreFuncVisitor::maybeEmitNote(PathSensitiveBugReport &R,
+                                  const CallEvent &Call, const ExplodedNode *N,
+                                  RegionPrinter RP) {
   // Optimistically suppress uninitialized value bugs that result
   // from system headers having a chance to initialize the value
   // but failing to do so. It's too unlikely a system header's fault.
   // It's much more likely a situation in which the function has a failure
   // mode that the user decided not to check. If we want to hunt such
   // omitted checks, we should provide an explicit function-specific note
   // describing the precondition under which the function isn't supposed to
   // initialize its out-parameter, and additionally check that such
NoStoreFuncVisitor::maybeEmitNote(PathSensitiveBugReport &R,
   if (!L.hasValidLocation())
     return nullptr;
 
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
   os << "Returning without writing to '";
 
   // Do not generate the note if failed to pretty-print.
-  if (!prettyPrintRegionName(FirstElement, FirstIsReferenceType, MatchedRegion,
-                             FieldChain, IndirectionLevel, os))
+  if (!RP.prettyPrintRegionName(os))
     return nullptr;
 
   os << "'";
   if (TKind == bugreporter::TrackingKind::Condition)
     os << WillBeUsedForACondition;
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
-bool NoStoreFuncVisitor::prettyPrintRegionName(StringRef FirstElement,
-                                               bool FirstIsReferenceType,
-                                               const MemRegion *MatchedRegion,
-                                               const RegionVector &FieldChain,
-                                               int IndirectionLevel,
-                                               llvm::raw_svector_ostream &os) {
+bool NoStoreFuncVisitor::RegionPrinter::prettyPrintRegionName(
 
   if (FirstIsReferenceType)
     IndirectionLevel--;
 
   RegionVector RegionSequence;
 
   // Add the regions in the reverse order, then reverse the resulting array.
   assert(RegionOfInterest->isSubRegionOf(MatchedRegion));
bool NoStoreFuncVisitor::RegionPrinter::prettyPrintRegionName(
   }
 
   if (Sep.empty())
     prettyPrintFirstElement(FirstElement,
                             /*MoreItemsExpected=*/false, IndirectionLevel, os);
   return true;
 }
 
-StringRef NoStoreFuncVisitor::prettyPrintFirstElement(
+StringRef NoStoreFuncVisitor::RegionPrinter::prettyPrintFirstElement(
     StringRef FirstElement, bool MoreItemsExpected, int IndirectionLevel,
     llvm::raw_svector_ostream &os) {
   StringRef Out = ".";
 
   if (IndirectionLevel > 0 && MoreItemsExpected) {
     IndirectionLevel--;
     Out = "->";
   }