[PATCH] D19644: [ThinLTO] Option to control path of distributed backend files

[Mehdi AMINI via llvm-commits]

joker.eph accepted this revision.
joker.eph added a comment.
This revision is now accepted and ready to land.

LGTM, with some suggestions inline (no need to review again)


================
Comment at: lib/Support/Path.cpp:533
@@ +532,3 @@
+  if (!OrigPath.startswith(OldPrefix))
+    return;
+
----------------
You mentioned as precondition in the doc:

```
 @param Path If \a Path starts with \a OldPrefix modify to instead
///        start with \a NewPrefix.
```

Here you're still doing the check. I'd either assert or update the comment.

================
Comment at: lib/Support/Path.cpp:539
@@ +538,3 @@
+  path::append(NewPath, RelPath);
+  Path.swap(NewPath);
+}
----------------
If `OldPrefix.size() == NewPrefix.size()` we have a possible fast-path in-place.
(the unittest should also be updated to test this case)

================
Comment at: tools/gold/gold-plugin.cpp:1217
@@ +1216,3 @@
+  StringRef PrefixReplace = options::thinlto_prefix_replace;
+  assert(PrefixReplace.empty() || PrefixReplace.find(":") != StringRef::npos);
+  std::pair<StringRef, StringRef> Split = PrefixReplace.split(":");
----------------
This is a user-supplied command line flag, if you assert he I'd expect it to be appropriately validated with a report_fatal_error (or equivalent) at parse time (line 226 above)


http://reviews.llvm.org/D19644