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

[Frédéric Riss]

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

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 <http://reviews.llvm.org/D6690>
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141218/81c8465a/attachment.html>