[PATCH] D151283: [llvm-cov] Support directory layout in coverage reports

[Yuhao Gu via Phabricator via llvm-commits]

AtomicGu marked 5 inline comments as done.
AtomicGu added inline comments.


================
Comment at: llvm/tools/llvm-cov/CoverageReport.cpp:559
+    const ArrayRef<StringRef> &Files, FileCoverageSummary *Totals) {
+  assert(!Files.empty() && "Only works when Files is not empty");
+
----------------
phosek wrote:
> Rather than using an `assert` which would be compiled out when assertions are disabled, I'd consider returning an `Error` in this case.
This assertion is to prevent coding errors. In a correctly written program, it cannot fail. All its callers, `prepareSubDirectoryReports` and `prepareDirectoryReports`, won't pass an empty `Files` to it. The only exception is that the `prepareDirectoryReports` is used incorrectly. So, I think returning an` Error` is unnecessary and might cause runtime overhead, potentially delaying the discovery of bugs.


================
Comment at: llvm/tools/llvm-cov/CoverageReport.h:130
+  /// relative path of the only file in that directory.
+  virtual Error operator()(SubFileReports &&SubFiles, SubDirReports &&SubDirs,
+                           FileCoverageSummary &&SubTotals) = 0;
----------------
phosek wrote:
> Is there a reason for using an operator here rather than a regular method?
No. I was thinking this class would be used like functions, but so far it's not. I'm going to rename it to `generateSubDirectoryReport`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151283/new/

https://reviews.llvm.org/D151283