[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.

This is looking good, there's just one real issue left with removeUnmappedInputs().



================
Comment at: test/tools/llvm-cov/scan-directory.test:11
 
-RUN: llvm-cov show /dev/null -instr-profile /dev/null -dump-collected-paths -filename-equivalence %t a b c d e f | FileCheck %s --check-prefix=EQUIV
+// NEEDS FIX.
+RUN: llvm-cov show /dev/null -instr-profile /dev/null -dump-collected-paths -path-equivalence=/tmp,%S %t a b c d e f | FileCheck %s --check-prefix=EQUIV
----------------
seaneveson wrote:
> I'm not sure what the expected output should be now. The actual output is:
> 
> ```
> error: a: Missing source file
> error: b: Missing source file
> error: c: Missing source file
> error: d: Missing source file
> error: e: Missing source file
> error: f: Missing source file
> D:\Test\x\a\b\c.tmp
> D:\Test\x\a\d.tmp
> ```
I think "remapped-from" should be "%t" (all of the 'source files' live within that directory, see the first RUN line). The "remapped-to" field can be whatever you'd like.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:822
+        if (NativeFilename.startswith(PathRemapping.first)) {
+          RemappedFilenames[Filename] =
+              PathRemapping.second +
----------------
seaneveson wrote:
> vsk wrote:
> > You should only need to call native() on the string Filename is mapped to.
> I think you do have to call native on both paths and the Filename to cover the case where the coverage data was generated on linux and llvm-cov is being run on windows (like in the lit tests).
> 
> Since the RemapFrom path and the Filename path are being compared it is important they are both native. Otherwise the RemapFrom path could be anything the user typed in, and we can't be sure about Filename because the data could come from another platform.
> 
> Given that the Filename path is then already native, it is quicker and easier to make RemapTo native and then just append the NativeFilename.
Ok, sgtm.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:127
   std::vector<std::string> SourceFiles;
 
+  /// In -path-equivalence mode, this maps absolute paths from the
----------------
Could you also remove the -filename-equivalance entry from docs/CommandGuide/llvm-cov.rst, and add in a short sentence or two about -path-equivalence?


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:355
   std::vector<StringRef> CoveredFiles = Coverage.getUniqueSourceFiles();
 
   auto UncoveredFilesIt = SourceFiles.end();
----------------
This function must ensure that if a file outside of the coverage mapping is passed to llvm-cov (outside of -filename-equivalence mode), it does not get displayed. With your change, the `!CompareFilenamesOnly` branch is still correct (the condition would need to be changed to `!PathRemapped`). It's the //other// branch which is now wrong: you'll need to do a reverse mapping on the source files, and check that coverage mapping contains it.


https://reviews.llvm.org/D36391