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

[Max Moroz via Phabricator via llvm-commits]

Dor1s marked 2 inline comments as done.
Dor1s added a comment.

Thanks for the comments, Vedant! I have some questions now :)



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


================
Comment at: tools/llvm-cov/CoverageSummaryInfo.h:181
 
+  FileCoverageSummary()
+      : Name(), RegionCoverage(), LineCoverage(), FunctionCoverage(),
----------------
vsk wrote:
> 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?
Yes :( I'm not sure how `make_unique` may help, e.g. if I do:

```
auto FileReports = std::make_unique<FileCoverageSummary[]>(Files.size());
```

It still needs a default constructor. Did you mean `llvm::make_unique` by any chance? It still looks for an empty constructor though


================
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.");
----------------
vsk wrote:
> If we stipulate that file summaries have valid names, we could avoid a bit of this complexity.
Sure!  But we still need to verify that `RHS.Name == LHS.Name`, right?


https://reviews.llvm.org/D41206