[analyzer][NFC] Refactor PlistDiagnostics to use a class instead of passing 9 parameters around

[Szelethus]

Summary

This has been a long time coming. Note the usage of AnalyzerOptions: I'll need it for D52742, and added it in rC343620. The main motivation for this was that I'll need to add yet another parameter to every single function, and some functions would reach their 10th (!!!!) parameter with that change.

[xazax.hun]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

82

The raw_ostream seems to be a common argument. MAybe it would be worth to make that a member too?

[Szelethus]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

82

I did that, and then later changed it back for debugging purposes, if anyone needs to print note pieces to errs or something.

[NoQ]

Wow thanks! Great point.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

82

I guess both problems could be solved by doing a private method raw_ostream while also wrapping it into a public method with a default raw_ostream.

[Szelethus]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

82

I think that would add unnecessary complication for very little value. I'd rather commit as is.

[NoQ]

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

82

Sure np, just thinking aloud.

[Szelethus]

Commited in rC345531. Oops.

lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

 #include "clang/Rewrite/Core/HTMLRewrite.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
+
 using namespace clang;
 using namespace ento;
 using namespace markup;
 
+//===----------------------------------------------------------------------===//
+// Declarations of helper classes and functions for emitting bug reports in
+// plist format.
+//===----------------------------------------------------------------------===//
+
 namespace {
   class PlistDiagnostics : public PathDiagnosticConsumer {
     const std::string OutputFile;
     const Preprocessor &PP;
     AnalyzerOptions &AnOpts;
     const bool SupportsCrossFileDiagnostics;
   public:
     PlistDiagnostics(AnalyzerOptions &AnalyzerOpts,
public:
     }
     bool supportsLogicalOpControlFlow() const override { return true; }
     bool supportsCrossFileDiagnostics() const override {
       return SupportsCrossFileDiagnostics;
     }
   };
 } // end anonymous namespace
 
-PlistDiagnostics::PlistDiagnostics(AnalyzerOptions &AnalyzerOpts,
-                                   const std::string& output,
-                                   const Preprocessor &PP,
-                                   bool supportsMultipleFiles)
-  : OutputFile(output), PP(PP), AnOpts(AnalyzerOpts),
-    SupportsCrossFileDiagnostics(supportsMultipleFiles) {}
+namespace {
 
-void ento::createPlistDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
-                                         PathDiagnosticConsumers &C,
-                                         const std::string& s,
-                                         const Preprocessor &PP) {
-  C.push_back(new PlistDiagnostics(AnalyzerOpts, s, PP,
-                                   /*supportsMultipleFiles*/ false));
+/// A helper class for emitting a single report.
+class PlistPrinter {
+  const FIDMap& FM;
+  AnalyzerOptions &AnOpts;
+  const Preprocessor &PP;
+
+public:
+  PlistPrinter(const FIDMap& FM, AnalyzerOptions &AnOpts,
+               const Preprocessor &PP)
+    : FM(FM), AnOpts(AnOpts), PP(PP) {
   }
 
-void ento::createPlistMultiFileDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
-                                                  PathDiagnosticConsumers &C,
-                                                  const std::string &s,
-                                                  const Preprocessor &PP) {
-  C.push_back(new PlistDiagnostics(AnalyzerOpts, s, PP,
-                                   /*supportsMultipleFiles*/ true));
+  void ReportDiag(raw_ostream &o, const PathDiagnosticPiece& P) {
+    ReportPiece(o, P, /*indent*/ 4, /*depth*/ 0, /*includeControlFlow*/ true);
+
+    // Don't emit a warning about an unused private field.
+    (void)AnOpts;
   }
 
-static void EmitRanges(raw_ostream &o,
-                       const ArrayRef<SourceRange> Ranges,
-                       const FIDMap& FM,
+private:
+  void ReportPiece(raw_ostream &o, const PathDiagnosticPiece &P,
+                   unsigned indent, unsigned depth, bool includeControlFlow,
+                   bool isKeyEvent = false) {
+    switch (P.getKind()) {
+      case PathDiagnosticPiece::ControlFlow:
+        if (includeControlFlow)
+          ReportControlFlow(o, cast<PathDiagnosticControlFlowPiece>(P), indent);
+        break;
+      case PathDiagnosticPiece::Call:
+        ReportCall(o, cast<PathDiagnosticCallPiece>(P), indent,
+                   depth);
+        break;
+      case PathDiagnosticPiece::Event:
+        ReportEvent(o, cast<PathDiagnosticEventPiece>(P), indent, depth,
+                    isKeyEvent);
+        break;
+      case PathDiagnosticPiece::Macro:
+        ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), indent, depth);
+        break;
+      case PathDiagnosticPiece::Note:
+        ReportNote(o, cast<PathDiagnosticNotePiece>(P), indent);
+        break;
+    }
+  }
+
+  void EmitRanges(raw_ostream &o, const ArrayRef<SourceRange> Ranges,
+                  unsigned indent);
+  void EmitMessage(raw_ostream &o, StringRef Message, unsigned indent);
+
+  void ReportControlFlow(raw_ostream &o,
+                         const PathDiagnosticControlFlowPiece& P,
+                         unsigned indent);
+  void ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
+                   unsigned indent, unsigned depth, bool isKeyEvent = false);
+  void ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P,
+                  unsigned indent, unsigned depth);
+  void ReportMacro(raw_ostream &o, const PathDiagnosticMacroPiece& P,
+                   unsigned indent, unsigned depth);
+  void ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
+                  unsigned indent);
+};
+
+} // end of anonymous namespace
+
+static void printBugPath(llvm::raw_ostream &o, const FIDMap& FM,
+                         AnalyzerOptions &AnOpts,
                          const Preprocessor &PP,
+                         const PathPieces &Path);
+
+/// Print coverage information to output stream {@code o}.
+/// May modify the used list of files {@code Fids} by inserting new ones.
+static void printCoverage(const PathDiagnostic *D,
+                          unsigned InputIndentLevel,
+                          SmallVectorImpl<FileID> &Fids,
+                          FIDMap &FM,
+                          llvm::raw_fd_ostream &o);
+//===----------------------------------------------------------------------===//
+// Methods of PlistPrinter.
+//===----------------------------------------------------------------------===//
+
+void PlistPrinter::EmitRanges(raw_ostream &o,
+                              const ArrayRef<SourceRange> Ranges,
                               unsigned indent) {
 
   if (Ranges.empty())
     return;
 
   Indent(o, indent) << "<key>ranges</key>\n";
   Indent(o, indent) << "<array>\n";
   ++indent;
 
   const SourceManager &SM = PP.getSourceManager();
   const LangOptions &LangOpts = PP.getLangOpts();
 
   for (auto &R : Ranges)
     EmitRange(o, SM,
               Lexer::getAsCharRange(SM.getExpansionRange(R), SM, LangOpts),
               FM, indent + 1);
   --indent;
   Indent(o, indent) << "</array>\n";
 }
 
-static void EmitMessage(raw_ostream &o, StringRef Message, unsigned indent) {
+void PlistPrinter::EmitMessage(raw_ostream &o, StringRef Message,
+                               unsigned indent) {
   // Output the text.
   assert(!Message.empty());
   Indent(o, indent) << "<key>extended_message</key>\n";
   Indent(o, indent);
   EmitString(o, Message) << '\n';
 
   // Output the short text.
   // FIXME: Really use a short string.
   Indent(o, indent) << "<key>message</key>\n";
   Indent(o, indent);
   EmitString(o, Message) << '\n';
 }
 
-static void ReportControlFlow(raw_ostream &o,
+void PlistPrinter::ReportControlFlow(raw_ostream &o,
                                      const PathDiagnosticControlFlowPiece& P,
-                              const FIDMap& FM,
-                              const Preprocessor &PP,
                                      unsigned indent) {
 
   const SourceManager &SM = PP.getSourceManager();
   const LangOptions &LangOpts = PP.getLangOpts();
 
   Indent(o, indent) << "<dict>\n";
   ++indent;
 
   Indent(o, indent) << "<key>kind</key><string>control</string>\n";
if (!s.empty()) {
     Indent(o, indent) << "<key>alternate</key>";
     EmitString(o, s) << '\n';
   }
 
   --indent;
   Indent(o, indent) << "</dict>\n";
 }
 
-static void ReportEvent(raw_ostream &o,
-                        const PathDiagnosticEventPiece& P,
-                        const FIDMap& FM,
-                        const Preprocessor &PP,
-                        unsigned indent,
-                        unsigned depth,
-                        bool isKeyEvent = false) {
+void PlistPrinter::ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P,
+                               unsigned indent, unsigned depth,
+                               bool isKeyEvent) {
 
   const SourceManager &SM = PP.getSourceManager();
 
   Indent(o, indent) << "<dict>\n";
   ++indent;
 
   Indent(o, indent) << "<key>kind</key><string>event</string>\n";
 
   if (isKeyEvent) {
     Indent(o, indent) << "<key>key_event</key><true/>\n";
   }
 
   // Output the location.
   FullSourceLoc L = P.getLocation().asLocation();
 
   Indent(o, indent) << "<key>location</key>\n";
   EmitLocation(o, SM, L, FM, indent);
 
   // Output the ranges (if any).
   ArrayRef<SourceRange> Ranges = P.getRanges();
-  EmitRanges(o, Ranges, FM, PP, indent);
+  EmitRanges(o, Ranges, indent);
 
   // Output the call depth.
   Indent(o, indent) << "<key>depth</key>";
   EmitInteger(o, depth) << '\n';
 
   // Output the text.
   EmitMessage(o, P.getString(), indent);
 
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
-static void ReportPiece(raw_ostream &o,
-                        const PathDiagnosticPiece &P,
-                        const FIDMap& FM, 
-                        const Preprocessor &PP,
-                        AnalyzerOptions &AnOpts,
-                        unsigned indent,
-                        unsigned depth,
-                        bool includeControlFlow,
-                        bool isKeyEvent = false);
-
-static void ReportCall(raw_ostream &o,
-                       const PathDiagnosticCallPiece &P,
-                       const FIDMap& FM,
-                       const Preprocessor &PP,
-                       AnalyzerOptions &AnOpts,
+void PlistPrinter::ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P,
                               unsigned indent,
                               unsigned depth) {
 
   if (auto callEnter = P.getCallEnterEvent())
-    ReportPiece(o, *callEnter, FM, PP, AnOpts, indent, depth,
-                /*includeControlFlow*/ true, P.isLastInMainSourceFile());
+    ReportPiece(o, *callEnter, indent, depth, /*includeControlFlow*/ true,
+                P.isLastInMainSourceFile());
 
 
   ++depth;
 
   if (auto callEnterWithinCaller = P.getCallEnterWithinCallerEvent())
-    ReportPiece(o, *callEnterWithinCaller, FM, PP, AnOpts, indent, depth,
+    ReportPiece(o, *callEnterWithinCaller, indent, depth,
                 /*includeControlFlow*/ true);
 
   for (PathPieces::const_iterator I = P.path.begin(), E = P.path.end();I!=E;++I)
-    ReportPiece(o, **I, FM, PP, AnOpts, indent, depth,
-                /*includeControlFlow*/ true);
+    ReportPiece(o, **I, indent, depth, /*includeControlFlow*/ true);
 
   --depth;
 
   if (auto callExit = P.getCallExitEvent())
-    ReportPiece(o, *callExit, FM, PP, AnOpts, indent, depth,
-                /*includeControlFlow*/ true);
+    ReportPiece(o, *callExit, indent, depth, /*includeControlFlow*/ true);
 }
 
-static void ReportMacro(raw_ostream &o,
+void PlistPrinter::ReportMacro(raw_ostream &o,
                                const PathDiagnosticMacroPiece& P,
-                        const FIDMap& FM,
-                        const Preprocessor &PP,
-                        AnalyzerOptions &AnOpts,
-                        unsigned indent,
-                        unsigned depth) {
+                               unsigned indent, unsigned depth) {
 
   for (PathPieces::const_iterator I = P.subPieces.begin(), E=P.subPieces.end();
        I!=E; ++I) {
-    ReportPiece(o, **I, FM, PP, AnOpts, indent, depth,
-                /*includeControlFlow*/ false);
+    ReportPiece(o, **I, indent, depth, /*includeControlFlow*/ false);
   }
 }
 
-static void ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
-                        const FIDMap& FM,
-                        const Preprocessor &PP,
+void PlistPrinter::ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P,
                               unsigned indent) {
 
   const SourceManager &SM = PP.getSourceManager();
 
   Indent(o, indent) << "<dict>\n";
   ++indent;
 
   // Output the location.
   FullSourceLoc L = P.getLocation().asLocation();
 
   Indent(o, indent) << "<key>location</key>\n";
   EmitLocation(o, SM, L, FM, indent);
 
   // Output the ranges (if any).
   ArrayRef<SourceRange> Ranges = P.getRanges();
-  EmitRanges(o, Ranges, FM, PP, indent);
+  EmitRanges(o, Ranges, indent);
 
   // Output the text.
   EmitMessage(o, P.getString(), indent);
 
   // Finish up.
   --indent;
   Indent(o, indent); o << "</dict>\n";
 }
 
-static void ReportDiag(raw_ostream &o,
-                       const PathDiagnosticPiece& P,
-                       const FIDMap& FM,
-                       const Preprocessor &PP,
-                       AnalyzerOptions &AnOpts) {
-  ReportPiece(o, P, FM, PP, AnOpts, /*indent*/ 4, /*depth*/ 0,
-              /*includeControlFlow*/ true);
-}
-
-static void ReportPiece(raw_ostream &o,
-                        const PathDiagnosticPiece &P,
-                        const FIDMap& FM,
-                        const Preprocessor &PP,
-                        AnalyzerOptions &AnOpts,
-                        unsigned indent,
-                        unsigned depth,
-                        bool includeControlFlow,
-                        bool isKeyEvent) {
-  switch (P.getKind()) {
-    case PathDiagnosticPiece::ControlFlow:
-      if (includeControlFlow)
-        ReportControlFlow(o, cast<PathDiagnosticControlFlowPiece>(P), FM, PP,
-                          indent);
-      break;
-    case PathDiagnosticPiece::Call:
-      ReportCall(o, cast<PathDiagnosticCallPiece>(P), FM, PP, AnOpts, indent,
-                 depth);
-      break;
-    case PathDiagnosticPiece::Event:
-      ReportEvent(o, cast<PathDiagnosticEventPiece>(P), FM, PP, indent, depth,
-                  isKeyEvent);
-      break;
-    case PathDiagnosticPiece::Macro:
-      ReportMacro(o, cast<PathDiagnosticMacroPiece>(P), FM, PP, AnOpts, indent,
-                  depth);
-      break;
-    case PathDiagnosticPiece::Note:
-      ReportNote(o, cast<PathDiagnosticNotePiece>(P), FM, PP, indent);
-      break;
-  }
-}
+//===----------------------------------------------------------------------===//
+// Static function definitions.
+//===----------------------------------------------------------------------===//
 
 /// Print coverage information to output stream {@code o}.
 /// May modify the used list of files {@code Fids} by inserting new ones.
 static void printCoverage(const PathDiagnostic *D,
                           unsigned InputIndentLevel,
                           SmallVectorImpl<FileID> &Fids,
                           FIDMap &FM,
                           llvm::raw_fd_ostream &o) {
for (auto I = ExecutedLines.begin(), E = ExecutedLines.end(); I != E; ++I) {
     Indent(o, IndentLevel) << "</array>\n";
   }
   IndentLevel--;
   Indent(o, IndentLevel) << "</dict>\n";
 
   assert(IndentLevel == InputIndentLevel);
 }
 
+static void printBugPath(llvm::raw_ostream &o, const FIDMap& FM,
+                         AnalyzerOptions &AnOpts,
+                         const Preprocessor &PP,
+                         const PathPieces &Path) {
+  PlistPrinter Printer(FM, AnOpts, PP);
+  assert(std::is_partitioned(
+           Path.begin(), Path.end(),
+           [](const std::shared_ptr<PathDiagnosticPiece> &E)
+             { return E->getKind() == PathDiagnosticPiece::Note; }) &&
+         "PathDiagnostic is not partitioned so that notes precede the rest");
+
+  PathPieces::const_iterator FirstNonNote = std::partition_point(
+      Path.begin(), Path.end(),
+      [](const std::shared_ptr<PathDiagnosticPiece> &E)
+        { return E->getKind() == PathDiagnosticPiece::Note; });
+
+  PathPieces::const_iterator I = Path.begin();
+
+  if (FirstNonNote != Path.begin()) {
+    o << "   <key>notes</key>\n"
+         "   <array>\n";
+
+    for (; I != FirstNonNote; ++I)
+      Printer.ReportDiag(o, **I);
+
+    o << "   </array>\n";
+  }
+
+  o << "   <key>path</key>\n";
+
+  o << "   <array>\n";
+
+  for (PathPieces::const_iterator E = Path.end(); I != E; ++I)
+    Printer.ReportDiag(o, **I);
+
+  o << "   </array>\n";
+}
+
+//===----------------------------------------------------------------------===//
+// Methods of PlistDiagnostics.
+//===----------------------------------------------------------------------===//
+
+PlistDiagnostics::PlistDiagnostics(AnalyzerOptions &AnalyzerOpts,
+                                   const std::string& output,
+                                   const Preprocessor &PP,
+                                   bool supportsMultipleFiles)
+  : OutputFile(output), PP(PP), AnOpts(AnalyzerOpts),
+    SupportsCrossFileDiagnostics(supportsMultipleFiles) {}
+
+void ento::createPlistDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
+                                         PathDiagnosticConsumers &C,
+                                         const std::string& s,
+                                         const Preprocessor &PP) {
+  C.push_back(new PlistDiagnostics(AnalyzerOpts, s, PP,
+                                   /*supportsMultipleFiles*/ false));
+}
+
+void ento::createPlistMultiFileDiagnosticConsumer(AnalyzerOptions &AnalyzerOpts,
+                                                  PathDiagnosticConsumers &C,
+                                                  const std::string &s,
+                                                  const Preprocessor &PP) {
+  C.push_back(new PlistDiagnostics(AnalyzerOpts, s, PP,
+                                   /*supportsMultipleFiles*/ true));
+}
 void PlistDiagnostics::FlushDiagnosticsImpl(
                                     std::vector<const PathDiagnostic *> &Diags,
                                     FilesMade *filesMade) {
   // Build up a set of FIDs that we use by scanning the locations and
   // ranges of the diagnostics.
   FIDMap FM;
   SmallVector<FileID, 10> Fids;
   const SourceManager& SM = PP.getSourceManager();
o << " <key>diagnostics</key>\n"
        " <array>\n";
 
   for (std::vector<const PathDiagnostic*>::iterator DI=Diags.begin(),
        DE = Diags.end(); DI!=DE; ++DI) {
 
     o << "  <dict>\n";
 
     const PathDiagnostic *D = *DI;
-    const PathPieces &Path = D->path;
-
-    assert(std::is_partitioned(
-             Path.begin(), Path.end(),
-             [](const std::shared_ptr<PathDiagnosticPiece> &E)
-               { return E->getKind() == PathDiagnosticPiece::Note; }) &&
-           "PathDiagnostic is not partitioned so that notes precede the rest");
-
-    PathPieces::const_iterator FirstNonNote = std::partition_point(
-        Path.begin(), Path.end(),
-        [](const std::shared_ptr<PathDiagnosticPiece> &E)
-          { return E->getKind() == PathDiagnosticPiece::Note; });
-
-    PathPieces::const_iterator I = Path.begin();
-
-    if (FirstNonNote != Path.begin()) {
-      o << "   <key>notes</key>\n"
-           "   <array>\n";
-
-      for (; I != FirstNonNote; ++I)
-        ReportDiag(o, **I, FM, PP, AnOpts);
-
-      o << "   </array>\n";
-    }
-
-    o << "   <key>path</key>\n";
-
-    o << "   <array>\n";
-
-    for (PathPieces::const_iterator E = Path.end(); I != E; ++I)
-      ReportDiag(o, **I, FM, PP, AnOpts);
-
-    o << "   </array>\n";
+    printBugPath(o, FM, AnOpts, PP, D->path);
 
     // Output the bug type and bug category.
     o << "   <key>description</key>";
     EmitString(o, D->getShortDescription()) << '\n';
     o << "   <key>category</key>";
     EmitString(o, D->getCategory()) << '\n';
     o << "   <key>type</key>";
     EmitString(o, D->getBugType()) << '\n';