[PATCH] D59491: Fix relative thin archive path handling

[Owen Reynolds via Phabricator via llvm-commits]

gbreynoo marked 2 inline comments as done.
gbreynoo added a comment.

The problem with working out the paths is if either are in the form `foo/../bar/`. I don't believe your above suggestion would work for paths with internal `../`.



================
Comment at: lib/Object/ArchiveWriter.cpp:499-500
 std::string computeArchiveRelativePath(StringRef From, StringRef To) {
-  if (sys::path::is_absolute(From) || sys::path::is_absolute(To))
-    return To;
+  auto getDotlessAbsolutePath =
+      [](SmallVectorImpl<char> &P) -> std::error_code {
+    std::error_code Ret = sys::fs::make_absolute(P);
----------------
ruiu wrote:
> This lambda doesn't capture anything, you probably should consider moving this outside of this function.
The lambda was to keep it local to this function, i'd rather keep it unless others really disagree. 


================
Comment at: lib/Object/ArchiveWriter.cpp:514-515
 
-  StringRef DirFrom = sys::path::parent_path(From);
-  auto FromI = sys::path::begin(DirFrom);
-  auto ToI = sys::path::begin(To);
+  SmallString<128> PathTo = To;
+  SmallString<128> DirFrom = sys::path::parent_path(From);
+  if (getDotlessAbsolutePath(PathTo) || getDotlessAbsolutePath(DirFrom))
----------------
ruiu wrote:
> Create instances of SmallString and populate their contents as a side-effect seems a bit awkward. You could instead make your helper function (which is currently named `getDotlessAbsolutePath`) to return an std::string.
Made a change as you've suggested but the use of SmallString is due to remove__dots() and I didn't want a second conversion to std::string. 





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

https://reviews.llvm.org/D59491