[PATCH] D151283: [WIP][llvm-cov] Support a Hierarchical Structure for `show --format=html` (WORK IN PROGRESS)

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem added inline comments.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageView.cpp:81
   case CoverageViewOptions::OutputFormat::HTML:
-    return std::make_unique<CoveragePrinterHTML>(Opts);
+    return std::make_unique<CoveragePrinterHTMLDirectory>(Opts);
   case CoverageViewOptions::OutputFormat::Lcov:
----------------
Some `llvm-cov` tests like `llvm-cov::branch-c-general.test` are failing in the presubmit tests. The reason is that you are changing the default behavior and some tests rely on that behavior. As a transitioning step, I would suggest adding a new option like `showDirectoryCoverage` into https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-cov/CoverageViewOptions.h#L30, and using the associated class based on that option like:

```
if (Opts.ShowDirectoryCoverage)
  return std::make_unique<CoveragePrinterHTMLDirectory>(Opts);
else
  return std::make_unique<CoveragePrinterHTML>(Opts); 
```


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:425
                                           const FileCoverageSummary &FCS,
-                                          bool IsTotals) const {
-  SmallVector<std::string, 8> Columns;
-
-  // Format a coverage triple and add the result to the list of columns.
-  auto AddCoverageTripleToColumn =
-      [&Columns, this](unsigned Hit, unsigned Total, float Pctg) {
-        std::string S;
-        {
-          raw_string_ostream RSO{S};
-          if (Total)
-            RSO << format("%*.2f", 7, Pctg) << "% ";
-          else
-            RSO << "- ";
-          RSO << '(' << Hit << '/' << Total << ')';
-        }
-        const char *CellClass = "column-entry-yellow";
-        if (Pctg >= Opts.HighCovWatermark)
-          CellClass = "column-entry-green";
-        else if (Pctg < Opts.LowCovWatermark)
-          CellClass = "column-entry-red";
-        Columns.emplace_back(tag("td", tag("pre", S), CellClass));
-      };
-
+                                          bool IsTotals) const {  
   // Simplify the display file path, and wrap it in a link if requested.
----------------
Please remove the whitespace at the end of this line. 


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:613
+  // std::map is used to keep entries in order.
+  std::map<StringRef, FileCoverageSummary> FilesHere;
+  std::map<StringRef, std::vector<StringRef>> DirsHere;
----------------
I would suggest using the words `CurrentFiles`, `CurrentDirs` and `CurrentTotals`.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:648
+    }
+
+    else {
----------------
Make sure that else starts after this curly base as stated in LLVM Coding Standards.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:711
+    LinkTargetStr = (RelPath + "index.html").str();
+  } else
+    LinkTargetStr = (RelPath + ".html").str();
----------------
Use a curly brace for the `else` part make it uniform.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:719
+
+std::string CoveragePrinterHTMLDirectory::buildNavLink(StringRef LCPath,
+                                                       unsigned LCP) const {
----------------
Can you use a long name here? What does `buildNavLink` mean? Try to use a full name unless the abbreviation is clear.


================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:875
       Snippets[I + 1] =
-          tag("div", Snippets[I + 1] + tag("span", formatCount(CurSeg->Count),
-                                           "tooltip-content"),
+          tag("div",
+              Snippets[I + 1] +
----------------
This is another formatting change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151283