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

[David Blaikie]

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


>
> ================
> 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/4fc23c4a/attachment.html>