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

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Support/Path.cpp:533
@@ +532,3 @@
+  if (!OrigPath.startswith(OldPrefix))
+    return;
+
----------------
joker.eph wrote:
> 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.
I think the code matches the description, which indicates that we only do the replacement if Path starts with OldPrefix. Here we are skipping the replacement if Path does not start with OldPrefix.

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

================
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(":");
----------------
joker.eph wrote:
> 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)
Ok

================
Comment at: tools/llvm-lto/llvm-lto.cpp:331
@@ +330,3 @@
+  return NewPath.str();
+}
+
----------------
joker.eph wrote:
> The duplication with the gold plugin isn't great for these two functions.
Agree, but I couldn't figure out a good way to avoid that. They didn't seem general enough to move into lib/Support either.


http://reviews.llvm.org/D19644