[PATCH] D36391: [llvm-cov] Add an option which maps the location of source directories on another machine to your local copies

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for the patch, this should address:
https://bugs.llvm.org/show_bug.cgi?id=30901



================
Comment at: tools/llvm-cov/CodeCoverage.cpp:137
+  /// remapped to, when using -path-equivalence.
+  std::pair<std::string, std::string> PathRemapping;
+
----------------
Please use Optional<std::pair<...>>, this will make it easier to see whether/not there is a remapping.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:520
 
   cl::opt<bool> FilenameEquivalence(
       "filename-equivalence", cl::Optional,
----------------
The path remap option is a strict improvement over -filename-equivalence. Could you delete the old option, delete CompareFilenamesOnly & any resulting dead code, and update any affected tests to use path remap?


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:614
+      PathRemapping.first = nativeWithTrailing(PathRemapping.first);
+      PathRemapping.second = nativeWithTrailing(PathRemapping.second);
+    } else
----------------
I don't think that either of these paths need to go through native(). That should happen right before inserting into RemappedFilenames.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:616
+    } else
+      PathRemapping = std::make_pair("", "");
+
----------------
The field can just be initialized to None in the CodeCoverageTool constructor.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:822
+        if (NativeFilename.startswith(PathRemapping.first)) {
+          RemappedFilenames[Filename] =
+              PathRemapping.second +
----------------
You should only need to call native() on the string Filename is mapped to.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:830
+    ViewOpts.colored_ostream(errs(), raw_ostream::RED)
+        << "path-equivalence was ignored as source files were given";
+  }
----------------
Why should -path-equivalence be ignored if source files are specified? IMO this should be supported so that users can see coverage for a restricted set of files.


https://reviews.llvm.org/D36391