[PATCH] D59491: Fix relative thin archive path handling

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Let P1 <https://reviews.llvm.org/P1> and P2 <https://reviews.llvm.org/P2> be pathnames. IIUC, the problem we want to solve is to how to compute a relative path from the last directory of P1 <https://reviews.llvm.org/P1> to P2 <https://reviews.llvm.org/P2>.  Is this correct?

If so, I guess the following scheme may be simpler than the current one:

1. Canonicalize both P1 <https://reviews.llvm.org/P1> and P2 <https://reviews.llvm.org/P2> by converting backslashes to slashes.
2. Remove common prefixes from P1 <https://reviews.llvm.org/P1> and P2 <https://reviews.llvm.org/P2>
3. Count the number of slashes in P1 <https://reviews.llvm.org/P1>. Let N be that number.
4. Concatenate "../../../../...." (repeated N times) and P2 <https://reviews.llvm.org/P2>. This is the result.

Here is an example when P1 <https://reviews.llvm.org/P1> = "foo/bar/baz/quux" and P2 <https://reviews.llvm.org/P2>="foo/x/y".

1. Canonicalize them (in this case this is a no-op)
2. P1 <https://reviews.llvm.org/P1> becomes "bar/baz/quux", P2 <https://reviews.llvm.org/P2> becomes "x/y".
3. N = 2
4. "../../x/y"



================
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);
----------------
This lambda doesn't capture anything, you probably should consider moving this outside of this function.


================
Comment at: lib/Object/ArchiveWriter.cpp:502
+    std::error_code Ret = sys::fs::make_absolute(P);
+    if (!Ret) {
+      sys::path::remove_dots(P, /*removedotdot*/ true);
----------------
Flip the condition and return early. `Err` would be a better name, as it can be read as "if error". Currently it is "if ret" which is a bit less readable.


================
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))
----------------
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.


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

https://reviews.llvm.org/D59491