[PATCH] D22651: Create llvm-cov structured export submodule

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thank you for working on this! I just have small nitpicks -- functionally, this patch looks like it's in great shape.

Could you add a short (1-2 line) description of the export command to `docs/CommandGuide/llvm-cov.rst`?


================
Comment at: test/tools/llvm-cov/Inputs/highlightedRanges.json:8
@@ +7,3 @@
+// File Object
+// CHECK-SAME: {"filename":"/Users/bogner/code/llvm/test/tools/llvm-cov/showHighlightedRanges.cpp",
+// CHECK-SAME: "segments":[
----------------
Can you use your shorter filename-matching regex for all filenames? Something like `{{[^\"]+}}showHighlightedRanges.cpp` in this case.

================
Comment at: test/tools/llvm-cov/Inputs/highlightedRanges.json:10
@@ +9,3 @@
+// CHECK-SAME: "segments":[
+// CHECK-SAME: [3,13,1,1,1],[5,3,0,1,1],[6,2,1,1,0],[6,2,0,0,0],
+// CHECK-SAME: [8,19,1,1,1],[9,13,1,1,1],[10,11,1,1,1],[10,17,1,1,0],
----------------
Here, it would be less brittle / more succinct to say:

`CHECK-SAME: {{(\[[0-9]+,[0-9]+,[0-9]+,[0-9]+,[0-9]+\],?)+}}`

We should do this for all arrays of regions/segments. If we need to change the format later on, it'll be much easier to do so this way. Plus, it's easier to check that a regex is correct vs. the actual region counts.

================
Comment at: test/tools/llvm-cov/Inputs/showExpansions.json:35
@@ +34,3 @@
+// CHECK-SAME: "filenames":[
+// CHECK-SAME: "{{[^\"]+}}/showExpansions.cpp","{{[^\"]+}}/showExpansions.cpp",
+// CHECK-SAME: "{{[^\"]+}}/showExpansions.cpp","{{[^\"]+}}/showExpansions.cpp"]
----------------
Why are there 4 of the same filenames? It sounds like a bug.. maybe fixed by clang/r275924 (remove '..' from filenames and normalize them)?

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:92
@@ +91,3 @@
+  /// \brief Emit a serialized scalar.
+  void emitSerialized(const int64_t &value) { OS << value; }
+
----------------
No reference needed here, since an int64_t is cheaper to copy than to deref+load. Also, `value` should be capitalized (this applies elsewhere).

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:199
@@ +198,3 @@
+           "All Elements In JSON were Closed");
+  };
+
----------------
Stray semicolon.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:202
@@ +201,3 @@
+  /// \brief Render an array of all the given functions.
+  void renderFunctions(iterator_range<FunctionRecordIterator>) {
+    // Start List of Functions.
----------------
Unused parameter. Please drop.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:243
@@ +242,3 @@
+      auto FileCoverage = Coverage.getCoverageForFile(SourceFile);
+      renderFile(std::move(FileCoverage));
+
----------------
Change `renderFile` to accept a `const CoverageData &`, and pass FileCoverage by reference. That's cheaper.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:264
@@ +263,3 @@
+
+    for (const auto Segment : FileCoverage) {
+      renderSegment(Segment);
----------------
This should be `const auto &`.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:265
@@ +264,3 @@
+    for (const auto Segment : FileCoverage) {
+      renderSegment(Segment);
+    }
----------------
Braces around single-statement loops are usually dropped.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:296
@@ +295,3 @@
+  /// \brief Render a CoverageSegment.
+  void renderSegment(CoverageSegment Segment) {
+    // Start Segment.
----------------
Should be `const CoverageSegment &`.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:345
@@ +344,3 @@
+    emitArrayEnd();
+  };
+
----------------
Stray semi-colon.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:348
@@ +347,3 @@
+  /// \brief Render a single CountedRegion.
+  void renderRegion(CountedRegion Region) {
+    // Start CountedRegion.
----------------
Should be `const CountedRegion &`.

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:366
@@ +365,3 @@
+  /// \brief Render a FileCoverageSummary.
+  void renderSummary(FileCoverageSummary Summary) {
+    // Start Summary for the file.
----------------
Should be `const FileCoverageSummary &`.


Repository:
  rL LLVM

https://reviews.llvm.org/D22651