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

Eddie Hurtig via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 22 15:28:49 PDT 2016


ehurtig marked 10 inline comments as done.

================
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"]
----------------
vsk wrote:
> Why are there 4 of the same filenames? It sounds like a bug.. maybe fixed by clang/r275924 (remove '..' from filenames and normalize them)?
Yeah I thought it was a bug at first, however, I have a feeling it is expected and has to do with nested expansions. The source for `showExpansions.cpp`:

```
#define DO_SOMETHING_ELSE() \
  do {                      \
  } while (0)
#define ANOTHER_THING() \
  do {                  \
    if (0) {            \
    }                   \
  } while (0)

#define DO_SOMETHING(x)    \
  do {                     \
    if (x)                 \
      DO_SOMETHING_ELSE(); \
    else                   \
      ANOTHER_THING();     \
  } while (0)
// CHECK-DAG: Expansion at line [[@LINE-4]], 7 -> 24
// CHECK-DAG: Expansion at line [[@LINE-3]], 7 -> 20

int main(int argc, const char *argv[]) {
  for (int i = 0; i < 100; ++i)
    DO_SOMETHING(i); // CHECK-DAG: Expansion at line [[@LINE]], 5 -> 17
  return 0;
}
```

0. FileID for the "Main" or "Source" file.
1. For the expansion of `DO_SOMETHING`
2. For the expansion of `ANOTHER_THING`
3. For the expansion of `DO_SOMETHING_ELSE`

Not saying whether it is right or wrong, just offering what I //think// is the reason for the duplicate filenames and separate FileIDs

================
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.
----------------
vsk wrote:
> Unused parameter. Please drop.
In order to match the rest of the `render*` functions, and specifically the `renderFiles(... list of files ...)` function, I have fixed the function to actually use this parameter and actually render the functions given to it. 

================
Comment at: tools/llvm-cov/CoverageExporterJson.cpp:243
@@ +242,3 @@
+      auto FileCoverage = Coverage.getCoverageForFile(SourceFile);
+      renderFile(std::move(FileCoverage));
+
----------------
vsk wrote:
> Change `renderFile` to accept a `const CoverageData &`, and pass FileCoverage by reference. That's cheaper.
There were some issues blocking this that are now resolved by some NFCs in r276474 and thus I have made the necessary changes to resolve this as well



Repository:
  rL LLVM

https://reviews.llvm.org/D22651





More information about the llvm-commits mailing list