[PATCH] D41206: [llvm-cov] Multi-threaded implementation of prepareFileReports method.
Vedant Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 20 11:32:23 PST 2017
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
More information about the llvm-commits
mailing list