[PATCH] [dsymutil] Implement the BinaryHolder object and gain archive support.

[David Blaikie]

On Thu, Dec 18, 2014 at 11:40 AM, Frédéric Riss <friss at apple.com> wrote:
>
>
> On Dec 18, 2014, at 11:35 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Dec 18, 2014 at 11:29 AM, Frederic Riss <friss at apple.com> wrote:
>>
>> ================
>> Comment at: tools/dsymutil/BinaryHolder.cpp:59
>> @@ +58,3 @@
>> +  if (!Filename.startswith(CurArchiveName) ||
>> +      Filename[CurArchiveName.size()] != '(')
>> +    return make_error_code(errc::no_such_file_or_directory);
>> ----------------
>> samsonov wrote:
>> > friss wrote:
>> > > samsonov wrote:
>> > > > You should check that Filename is long enough.
>> > > If Filename startsWith CurArchiveName, then
>> FileName[CurArchiveName.size()] is valid because these StringRefs come from
>> std::strings that are null terminated.
>> > Right, but if StringRef is a type of an argument in your public
>> interface, it's better not to depend on the fact that these StringRef's
>> will come from a particular source.
>> Makes sense, I'll add the check.
>>
>
> I don't think you actually need the check - if Filename startsWith
> CurAchiveName, then Filename must be at least CurArchiveName.size() long,
> and std::string's operator[] is defined (in C++11) for the case where the
> index == size():
>
> http://en.cppreference.com/w/cpp/string/basic_string/operator_at
>
> You are guaranteed that if you do str[str.size()] you'll get '\0', which
> is fine for your pusrposes. If I'm understanding this all correctly…
>
>
> Well that was my reasoning for objecting the change, but Alexey is right
> that although the current use passes std::strings, it’s not nice to rely on
> it in the API. Someone could one day pass in a StringRef that doesn’t
> originate from a std::string.
>

Ah, sorry - I'd assumed Filename was actually a std::string and
CurAchiveName was the StringRef. My mistake. Carry on.

- David


>
> Fred
>
> ================
>> Comment at: tools/dsymutil/BinaryHolder.cpp:78
>> @@ +77,3 @@
>> +BinaryHolder::MapArchiveAndGetMemberBuffer(StringRef Filename) {
>> +  StringRef ArchiveFilename = Filename.substr(0, Filename.find('('));
>> +
>> ----------------
>> samsonov wrote:
>> > friss wrote:
>> > > samsonov wrote:
>> > > > Hm, don't you need to take the last opening parenthesis?
>> > > Not here, we are extracting the archive name. When we extract the
>> member name in GetArchiveMemberBuffer we drop the last character.
>> > I mean last *opening* parenthesis. Can we have a Filename:
>> >   my(small)archive.a(member.o)
>> >
>> Sorry for the confusion. Unfortunately, I think there's no nice way to
>> handle this. The member could very well have an opening parenthesis in its
>> name, making the last opening parenthesis the wrong one too. An option
>> would be to try every parenthesis in the filename in turn, but I'd rather
>> leave that out of this patch (not sure we ever want to handle it).
>>
>> http://reviews.llvm.org/D6690
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141218/1e96e229/attachment.html>