[PATCH] D56769: Path: enhance prefix mapping

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn added inline comments.


================
Comment at: lib/Support/Path.cpp:544
+      OrigPath.size() > OldPrefixDir.size())
+    return false;
+
----------------
Suggestion: guard all of the previous code in `if (!OldPrefix.empty()) {`.

Use `OrigPath.size() > OldPrefixDir.size() && !is_separator(OrigPath[OldPrefixDir.size()], style)` to avoid an out-of-bounds read. (`operator[Index]` requires `Index < Length`.)

If you build LLVM with assertions enabled, this should have tripped.


================
Comment at: lib/Support/Path.cpp:550
+    return true;
   }
 
----------------
This does not handle all substitutions:

- `/old`, `/new` - handled
- `/old/`, `/new` - handled
- `/old/`, `/new/` - not handled - should be accepted (old and new prefix are of the same length)
- `/old`, `/new/` - not handled - should be accepted. `OrigPath` started with `/old` and followed by `/`, so it is safe to overwrite it. One exception: if `OrigPath` was `/old`, then the new buffer is too short, so it needs a check for `OrigPath.size() >= `NewPrefix.size()`.

Observe that the only the length is necessary, so you do not have to copy the string contents. Something like:
```
size_t NewPrefixDirLength = NewPrefix.size();

if (!NewPrefix.empty() && is_separator(NewPrefix.back(), style))
  --NewPrefixDirLength;
// ...
if (OldPrefixDir.size() == NewPrefixDirLength) {
```


================
Comment at: lib/Support/Path.cpp:558
+  else
+    path::append(NewPath, style, relative_path(RelPath, style));
   Path.swap(NewPath);
----------------
`RelPath[0]` could be invalid if `OldPath` is the same as `OldPrefix`.

Is the `relative_path` call necessary? What happens if you call `path::append(NewPath, "/foo")`? Will it produce `/foo` or `<NewPath>/foo`?

If NewPrefix is empty, will this correctly avoid the leading directory separator?


================
Comment at: unittests/Support/Path.cpp:1232
   path::replace_path_prefix(Path, OldPrefix, EmptyPrefix);
-  EXPECT_EQ(Path, "/foo");
+  EXPECT_EQ(Path, "foo");
+  Path = Path3;
----------------
Is it desired to always turn paths into absolute paths into relative when the prefix matches? This test currently checks:
- /old/foo, /old, "" => foo

The old behavior was:
- /old/foo, /old, "" => /foo
(consider: `-ffile-prefix-map=/buildroot=` for `/buildroot/usr/src/foo.c`)

If this is really what you want, then users now have to use:
- /old/foo, /old, /
- /old/foo, /old/, /


================
Comment at: unittests/Support/Path.cpp:1238
+  path::replace_path_prefix(Path, OldPrefixSep, NewPrefix);
+  EXPECT_EQ(Path, "/oldnew/foo");
+  Path = Path1;
----------------
This case is already covered by the previous test (which rejects a partial prefix:
- /oldnew/foo, /old, /new

What about removing that test and adding these instead:
- /old/foo, /old/, "" => "foo" (Path2, OldPrefixSep, EmptyPrefix)
- /old/foo, /old, "/" => "/foo" (Path2, OldPrefix, ...)

And perhaps to exercise replacements by a non-root directory:
- /old/foo, /old/, /longernew => "/longernew/foo" (Path2, OldPrefixSep, NewPrefix2)
- /old/foo, /old, /longernew => "/longernew/bar" (Path2, OldPrefix, NewPrefix2)
- /old/foo, /old, /longernew/ => "/longernew/bar" (Path2, OldPrefix, ...)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56769