<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Looks fine to me.<div class=""><br class=""></div><div class="">Kev</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 28, 2017, at 3:49 PM, Alexander Shaposhnikov <<a href="mailto:alexander.v.shaposhnikov@gmail.com" class="">alexander.v.shaposhnikov@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">Hi, </span></font><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">many thanks for looking at the diff.</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">(i started working on this because this interface change broke some out-of-tree code, but that's expected and not a big issue, i just wanted to clean it up a bit). </span></font></div><div class=""><span style="color:rgb(51,51,51);font-family:"normal arial",sans-serif;font-size:16px" class=""><br class=""></span></div><div class=""><span style="color:rgb(51,51,51);font-family:"normal arial",sans-serif;font-size:16px" class="">I assume I might be missing smth, </span></div><div class=""><span style="color:rgb(51,51,51);font-family:"normal arial",sans-serif;font-size:16px" class="">however my diff doesn't change the static method </span><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class=""> </span></font></div><div class=""><span style="font-size:16px;color:rgb(51,51,51);font-family:"normal arial",sans-serif" class="">/// For use examining a trie not in a MachOObjectFile.</span><br class=""></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">static iterator_range<export_iterator> exports(Error &Err,</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class=""> ArrayRef<uint8_t> Trie,</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class=""> const MachOObjectFile *O =</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class=""> nullptr);</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class=""><br class=""></span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">It changes only the regular method </span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">/// For use iterating over all exported symbols.</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">iterator_range<export_iterator> exports(Error &Err, </span></font><span style="color:rgb(51,51,51);font-family:"normal arial",sans-serif;font-size:16px" class="">const MachOObjectFile *O</span><span style="font-size:16px;color:rgb(51,51,51);font-family:"normal arial",sans-serif" class="">) const;</span></div><div class=""><span style="font-size:16px;color:rgb(51,51,51);font-family:"normal arial",sans-serif" class="">because I didn't like syntax Obj->exports(Err, Obj).</span></div><div class=""><span style="font-size:16px;color:rgb(51,51,51);font-family:"normal arial",sans-serif" class="">LLD builds successfully with my patch - so I am wondering if this change looks reasonable/safe to you.</span></div><div class=""><span style="font-size:16px;color:rgb(51,51,51);font-family:"normal arial",sans-serif" class=""> </span><br class=""></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">Thanks,</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class="">Alex Shaposhnikov</span></font></div><div class=""><font color="#333333" face="normal arial, sans-serif" class=""><span style="font-size:16px" class=""><br class=""></span></font></div><div class=""><br class=""></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Jul 28, 2017 at 10:30 AM, David Blaikie <span dir="ltr" class=""><<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><span class=""><div dir="ltr" class="">On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby <<a href="mailto:enderby@apple.com" target="_blank" class="">enderby@apple.com</a>> wrote:<br class=""></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" class=""><div class=""><blockquote type="cite" class=""><div class="">On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank" class="">reviews@reviews.llvm.org</a>> wrote:</div><br class="m_5464274064247327764m_1946500766084757489Apple-interchange-newline"><div class=""><div class="">compnerd added a comment.<br class=""><br class="">Does anyone use the overload with the `O` for `exports` with `nullptr` instead of `this`? If not, we could just inline `this` throughout.<br class=""></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap:break-word" class=""><div class="">This will break lld as it needs the default nullptr. See <span style="font-family:'Helvetica Neue'" class="">r308691.</span></div><div class=""><span style="font-family:'Helvetica Neue'" class=""><br class=""></span></div><div class=""><span style="font-family:'Helvetica Neue'" class="">The reason that O was added was so that this check from r</span>308690 could be added.</div></div></blockquote></span><div class=""><br class="">When O is specified is it always == this, though? That seems to be implied by the original post.<br class=""><br class="">If that's the case, then the ability to pass null is really only passing the on/off state for the diagnostic? Is that necessary, or would it be good to just always produce the Error, even in lld?<br class=""> </div><div class=""><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><span style="font-family:'Helvetica Neue'" class=""><br class=""></span></div><div class="">+ if (O != nullptr) {<br class="">+ if (State.Other > O->getLibraryCount()) {<br class="">+ *E = malformedError("bad library ordinal: " + Twine((int)State.Other)<br class="">+ + " (max " + Twine((int)O->getLibraryCount(<wbr class="">)) + ") in export "<br class="">+ "trie data at node: 0x" + utohexstr(offset));<br class="">+ moveToEnd();<br class="">+ return;<br class="">+ }</div><div class=""><br class=""></div><div class="">This is needed for the test case:</div><div class=""><br class=""></div><div class="">+RUN: not llvm-objdump -macho -exports-trie %p/Inputs/macho-trie-bad-<wbr class="">library-ordinal 2>&1 | FileCheck -check-prefix BAD_LIBRARY_ORDINAL %s <br class="">+BAD_LIBRARY_ORDINAL: macho-trie-bad-library-<wbr class="">ordinal': truncated or malformed object (bad library ordinal: 69 (max 3) in export trie data at node: 0x33)</div></div><div style="word-wrap:break-word" class=""><div class=""><font face="Helvetica Neue" class=""><br class=""></font><blockquote type="cite" class=""><div class=""><div class=""><br class=""><br class=""><br class="">================<br class="">Comment at: tools/llvm-nm/llvm-nm.cpp:1230<br class=""> Error Err = Error::success();<br class="">- for (const llvm::object::ExportEntry &Entry : MachO->exports(Err,<br class="">- <wbr class=""> <wbr class=""> MachO)) {<br class="">+ for (const llvm::object::ExportEntry &Entry : MachO->exports(Err)) {<br class=""> bool found = false;<br class="">----------------<br class="">I think that using `auto` here instead of `llvm::object:ExportEntry` is better for readability.<br class=""><br class=""><br class="">================<br class="">Comment at: tools/llvm-objdump/MachODump.<wbr class="">cpp:9406<br class=""> Error Err = Error::success();<br class="">- for (const llvm::object::ExportEntry &Entry : Obj->exports(Err, Obj)) {<br class="">+ for (const llvm::object::ExportEntry &Entry : Obj->exports(Err)) {<br class=""> uint64_t Flags = Entry.flags();<br class="">----------------<br class="">Similar.<br class=""><br class=""><br class="">Repository:<br class=""> rL LLVM<br class=""><br class=""><a href="https://reviews.llvm.org/D35961" target="_blank" class="">https://reviews.llvm.org/<wbr class="">D35961</a><br class=""><br class=""><br class=""><br class=""></div></div></blockquote></div><br class=""></div></blockquote></div></div></div></div>
</blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></body></html>