[PATCH] D56769: Path: enhance prefix mapping

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn added inline comments.


================
Comment at: include/llvm/Support/Path.h:157
 ///   /old/foo, /old, <empty> => /foo
 /// @endcode
 ///
----------------
Perhaps these examples can be extended with special treatment of the trailing slash?

```
/old/foo, /old/, /new/ -> /new/foo
/old/foo, /old/, /new -> /new/foo         // looks more desirable than /newfoo I guess?
/old/foo, /old, /new/ -> /new/foo         // looks more desirable than /new//foo I guess?
/old/foo, /old/, <empty> -> foo           // better than /foo?
```

The last feature is most important to me for clean stack traces (`/srcdir/subfdir/foo.c` + `-ffile-prefix-map=/srcdir/=` -> `subdir/foo.c`, use `dir /new/srcdir` in gdb to enable proper debugging anyway).


================
Comment at: lib/Support/Path.cpp:537
+    Prefix = OldPrefix;
+  }
+
----------------
Coding style: remove curly braces from if/else.

Could you also rename `Prefix` to `OldPrefixDir` to make the meaning obvious? (I stopped further review here though since the specification of this function is not yet clear.)


================
Comment at: unittests/Support/Path.cpp:1213
   SmallString<64> OldPrefix("/old");
+  SmallString<64> OldPrefix2("/old/");
   SmallString<64> NewPrefix("/new");
----------------
Maybe call this `OldPrefixSep` or `OldPrefixSlash` to signify the relation between the two prefixes?


================
Comment at: unittests/Support/Path.cpp:1247
+  path::replace_path_prefix(Path, OldPrefix, NewPrefix);
+  EXPECT_EQ(Path, "/new/");
 }
----------------
Another corner case to check:
```
Path = OldPrefix; // /old
path::replace_path_prefix(Path, OldPrefix, NewPrefix); // /old/ -> /new
EXPECT_EQ(Path, "/old"); // or should this be /new?
```


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56769/new/

https://reviews.llvm.org/D56769