[PATCH] D56769: Path: enhance prefix mapping

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn requested changes to this revision.
Lekensteyn added a comment.
This revision now requires changes to proceed.

This has some broken edge cases. Consider extending the `ReplacePathPrefix` test in unittests/Support/Path.cpp for the new cases.



================
Comment at: lib/Support/Path.cpp:532
 
+  if ((is_separator(OldPrefix.back(), style) &&
+       !OrigPath.startswith(parent_path(OldPrefix, style))) ||
----------------
Wouldn't this crash if `OldPrefix` is empty? Both if blocks should probably be wrapped in `if (!OldPrefix.empty()) {`


================
Comment at: lib/Support/Path.cpp:533
+  if ((is_separator(OldPrefix.back(), style) &&
+       !OrigPath.startswith(parent_path(OldPrefix, style))) ||
+      (!is_separator(OldPrefix.back(), style) &&
----------------
Should return false for Path="/foobar" and OldPrefix="/foo/", but it does not.


================
Comment at: lib/Support/Path.cpp:552
   path::append(NewPath, style, NewPrefix);
-  path::append(NewPath, style, RelPath);
+  if (!is_separator(RelPath[0], style))
+    path::append(NewPath, style, RelPath);
----------------
If `OldPrefix` is empty, then this could potentially contain `C:\foo` I guess.


================
Comment at: lib/Support/Path.cpp:557
+  while (is_separator(NewPath.back(), style))
+    NewPath.set_size(NewPath.size() - 1);
   Path.swap(NewPath);
----------------
Trimming *all* slashes is probably beyond the scope for this function.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56769