[PATCH] D19644: [ThinLTO] Option to control path of distributed backend files
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Mon May 16 21:47:53 PDT 2016
tejohnson added inline comments.
Comment at: lib/Support/Path.cpp:533
@@ +532,3 @@
+ if (!OrigPath.startswith(OldPrefix))
> 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);
> 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(":");
> 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)
Comment at: tools/llvm-lto/llvm-lto.cpp:331
@@ +330,3 @@
+ return NewPath.str();
> 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.
More information about the llvm-commits