[PATCH] D41206: [llvm-cov] Multi-threaded implementation of prepareFileReports method.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Nice!

For a test, I suggest creating a report for >1 files with/without threading, and asserting that the reports are identical. It would be good to exercise the new summary aggregation logic by including different instantiations of the same template function in >1 files.



================
Comment at: tools/llvm-cov/CoverageReport.cpp:370
+  ThreadPool Pool(NumThreads);
+  std::vector<FileCoverageSummary> PartialTotals(Files.size());
+  std::vector<FileCoverageSummary> FileReports(Files.size());
----------------
IIUC, PartialTotals should contain exactly the same results as the FileReports vector after Pool.wait(). Why not get rid of it?


================
Comment at: tools/llvm-cov/CoverageReport.cpp:381
+
+  for (const auto& TotalsPart : PartialTotals)
+    Totals += TotalsPart;
----------------
clang-format?


================
Comment at: tools/llvm-cov/CoverageReport.h:48
+  static void
+  prepareSingleFileReport(const StringRef &Filename,
+                          const coverage::CoverageMapping *Coverage,
----------------
Since StringRef is immutable and cheap to copy, could you pass it by value?


================
Comment at: tools/llvm-cov/CoverageSummaryInfo.h:181
 
+  FileCoverageSummary()
+      : Name(), RegionCoverage(), LineCoverage(), FunctionCoverage(),
----------------
I don't think we should make it possible to create file summaries with empty names. Was this introduced so the std::vector could default-initialize FileCoverageSummary? If so, would make_unique<FileCoverageSummary[]>(N) suffice as an alternative?


================
Comment at: tools/llvm-cov/CoverageSummaryInfo.h:190
+  FileCoverageSummary &operator+=(const FileCoverageSummary &RHS) {
+    assert((RHS.Name.empty() || RHS.Name == LHS.Name) &&
+           "Addition of file coverage summary from different files.");
----------------
If we stipulate that file summaries have valid names, we could avoid a bit of this complexity.


https://reviews.llvm.org/D41206