[PATCH] D59491: Fix relative thin archive path handling

[Owen Reynolds via llvm-commits]

Thanks for the quick response Rui. To clarify, I agree with the scheme you
suggest with a call to remove_dots(). I also consider conversion to
absolute paths a requirement so the path roots can be compared. This
implementation was based of the existing use of path iterators when taking
the above into account, is there an alternate implementation you would
prefer?

On Wed, Mar 20, 2019 at 5:57 PM Rui Ueyama <ruiu at google.com> wrote:

> On Wed, Mar 20, 2019 at 10:54 AM Owen Reynolds via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> 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 `../`.
>>
>
> You can easily remove them using remove_dots().
>
>
>>
>>
>> ================
>> 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
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190321/16c778f0/attachment.html>