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


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:179
 void CodeCoverageTool::addCollectedPath(const std::string &Path) {
-  if (CompareFilenamesOnly) {
+  if (PathRemapping) {
     SourceFiles.emplace_back(Path);
----------------
I don't think we need the `PathRemapping` check here (you can just keep the else branch). Otherwise, using relative paths in path equivalence mode won't work.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:368
+  if (PathRemapping) {
+    // Convert to remapping paths to native paths with trailing seperators.
+    auto nativeWithTrailing = [](StringRef path) -> std::string {
----------------
nit, extra 'to'


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:379
+                                : nativeWithTrailing(PathRemapping->first);
+    std::string RemapTo = nativeWithTrailing(PathRemapping->second);
+
----------------
I can see why 'remap-from' would be empty (the user wants to add a path prefix). It would also make sense if 'remap-to' were empty (the user wants to strip out a path prefix).

Maybe the emptiness check should be in nativeWithTrailing?


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:405
   } else {
-    for (auto &SF : SourceFiles) {
-      StringRef SFBase = sys::path::filename(SF);
-      for (const auto &CF : CoveredFiles) {
-        if (SFBase == sys::path::filename(CF)) {
-          RemappedFilenames[CF] = SF;
-          SF = CF;
-          break;
-        }
-      }
+    StringMap<std::string> InvRemappedFilenames;
+    for (const auto &RemappedFilename : RemappedFilenames)
----------------
I don't see the need for this side table. It should be possible to do this without using any extra space:

```
std::remove_if(SourceFiles.begin(), SourceFiles.end(), [&](const std::string &filename) {
  if (filename.startswith(PathRemapping.second))
    inv_mapped_path = path::join(PathRemapping.first, full_path.drop_first(PathRemapping.second.size()))
  else
    inv_mapped_path = filename
  return !std::binary_search(CoveredFiles.begin(), CoveredFiles.end(), inv_mapped_path);
});
```

This might fit nicely in the previous call to remove_if().


https://reviews.llvm.org/D36391