[PATCH] D151283: [llvm-cov] Support a Hierarchical Structure for HTML Coverage Report Generating

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/test/tools/llvm-cov/directory_coverage.test:1
+// RUN: mkdir -p %t && clang -fprofile-instr-generate=%t/default.profraw -fcoverage-mapping -o %t/main.out \
+// RUN:   %S/Inputs/directory_coverage/main.cc \
----------------
Your test cannot assume that `clang` is available on the host since Clang is not one of LLVM's prerequisites, and we want LLVM to remain compiler agnostic. That's why you're seeing the issue on Windows pre-merge builders: Clang happens to be available on these builders, but it doesn't include the profile runtime, which is perfectly valid because we don't require it.

What we do instead is to either include raw profile in the `.proftext` textual format and then convert it to `.profdata` using `llvm-profdata` or include `.profdata` directly, plus include coverage mapping information in the textual format as `.covmapping`, and then run `llvm-cov`as part of the test. We also include instructions for recreating the `.profdata` and `.covmapping` files inside the test.

Here are some examples as an inspiration:

https://github.com/llvm/llvm-project/blob/96ae0851c26237378fa1280b0a9ad713e1b72bdb/llvm/test/tools/llvm-cov/coverage-prefix-map.test
https://github.com/llvm/llvm-project/blob/96ae0851c26237378fa1280b0a9ad713e1b72bdb/llvm/test/tools/llvm-cov/multiple-objects.test



================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:432
+
+void CoveragePrinterHTML::emitBeforeTable(raw_ostream &OSRef,
+                                          const std::string &Title) {
----------------
AtomicGu wrote:
> phosek wrote:
> > This is a just a suggestion, but I think that `emitPrelude` might be a better name.
> This name has been used by another function in the anonymous namespace in `SourceCoverageViewHTML.cpp`. So I chose a different name to avoid confusion, although no conflict would be caused if I used `emitPrelude` since the arguments are different.
I see, how about `emitReportHeader`?


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:607-608
+  // std::map is used to keep entries in order.
+  std::map<StringRef, FileCoverageSummary> CurrentFiles;
+  std::map<StringRef, std::vector<StringRef>> CurrentDirs;
+  for (auto &&File : Files) {
----------------
LLVM's `StringMap` is more efficient than `std::map<StringRef, ...>`.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:608
+  std::map<StringRef, FileCoverageSummary> CurrentFiles;
+  std::map<StringRef, std::vector<StringRef>> CurrentDirs;
+  for (auto &&File : Files) {
----------------
We prefer LLVM `SmallVector` unless there's a specific reason for using `std::vector`.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:697-708
+  auto Iter = LCPStack.begin(), IterE = LCPStack.end();
+  if (*Iter > 0)
+    Components.push_back({
+        LCPath.substr(0, *Iter - 1).str(), // Remove trailing slash.
+        0,
+    });
+  for (auto Last = *Iter; ++Iter != IterE; Last = *Iter) {
----------------
Can you add some comments to explain the logic in this block?


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:710
+
+  std::string S;
+  for (auto I = Components.begin(), E = Components.end();;) {
----------------
Use `llvm::SmallString`.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:714-715
+    if (++I == E) {
+      S += a("./index.html", Name);
+      S += sys::path::get_separator();
+      break;
----------------
Use `llvm::sys::path::append` appending to a path.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:737
+
+  std::string LinkTargetStr;
+  if (IsDir) {
----------------
Use `llvm::SmallString` here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151283