<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 18, 2014 at 11:40 AM, Frédéric Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><span class=""><div>On Dec 18, 2014, at 11:35 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br></span><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Dec 18, 2014 at 11:29 AM, Frederic Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>================<br>
Comment at: tools/dsymutil/BinaryHolder.cpp:59<br>
@@ +58,3 @@<br>
+  if (!Filename.startswith(CurArchiveName) ||<br>
+      Filename[CurArchiveName.size()] != '(')<br>
+    return make_error_code(errc::no_such_file_or_directory);<br>
----------------<br>
</span><span>samsonov wrote:<br>
> friss wrote:<br>
> > samsonov wrote:<br>
> > > You should check that Filename is long enough.<br>
> > If Filename startsWith CurArchiveName, then FileName[CurArchiveName.size()] is valid because these StringRefs come from std::strings that are null terminated.<br>
> 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.<br>
</span>Makes sense, I'll add the check.<br></blockquote></span><div><span class=""><br>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():<br><br><a href="http://en.cppreference.com/w/cpp/string/basic_string/operator_at" target="_blank">http://en.cppreference.com/w/cpp/string/basic_string/operator_at</a><br><br></span>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… <br></div></div></div></div></div></blockquote><div><br></div><div>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.</div></div></div></blockquote><div><br>Ah, sorry - I'd assumed Filename was actually a std::string and CurAchiveName was the StringRef. My mistake. Carry on.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>Fred</div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>
================<br>
Comment at: tools/dsymutil/BinaryHolder.cpp:78<br>
@@ +77,3 @@<br>
+BinaryHolder::MapArchiveAndGetMemberBuffer(StringRef Filename) {<br>
+  StringRef ArchiveFilename = Filename.substr(0, Filename.find('('));<br>
+<br>
----------------<br>
</span><span>samsonov wrote:<br>
> friss wrote:<br>
> > samsonov wrote:<br>
> > > Hm, don't you need to take the last opening parenthesis?<br>
> > Not here, we are extracting the archive name. When we extract the member name in GetArchiveMemberBuffer we drop the last character.<br>
> I mean last *opening* parenthesis. Can we have a Filename:<br>
>   my(small)archive.a(member.o)<br>
><br>
</span>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).<br>
<div><div><br>
<a href="http://reviews.llvm.org/D6690" target="_blank">http://reviews.llvm.org/D6690</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div></div></div>
</div></blockquote></span></div><br></div></blockquote></div></div></div>