<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 28, 2017, at 10:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby <<a href="mailto:enderby@apple.com" 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_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><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></div></div></blockquote><div><br class=""></div>Without O one can’t use O->getLibraryCount() which is what is needed for the test. I guess one could change the interface if lld really wanted this check. But I’d leave that to the lld folks as to what level of error checking they want.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div 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=""><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()) + ") 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-library-ordinal 2>&1 | FileCheck -check-prefix BAD_LIBRARY_ORDINAL %s <br class="">+BAD_LIBRARY_ORDINAL: macho-trie-bad-library-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="">- 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.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/D35961</a><br class=""><br class=""><br class=""><br class=""></div></div></blockquote></div><br class=""></div></blockquote></div></div>
</div></blockquote></div><br class=""></body></html>