Looks fine to me. Kev
> On Jul 28, 2017, at 3:49 PM, Alexander Shaposhnikov > <alexander.v.shaposhni...@gmail.com> wrote: > > Hi, > many thanks for looking at the diff. > (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). > > I assume I might be missing smth, > however my diff doesn't change the static method > /// For use examining a trie not in a MachOObjectFile. > static iterator_range<export_iterator> exports(Error &Err, > ArrayRef<uint8_t> Trie, > const MachOObjectFile *O = > > nullptr); > > It changes only the regular method > /// For use iterating over all exported symbols. > iterator_range<export_iterator> exports(Error &Err, const MachOObjectFile *O) > const; > because I didn't like syntax Obj->exports(Err, Obj). > LLD builds successfully with my patch - so I am wondering if this change > looks reasonable/safe to you. > > Thanks, > Alex Shaposhnikov > > > > On Fri, Jul 28, 2017 at 10:30 AM, David Blaikie <dblai...@gmail.com > <mailto:dblai...@gmail.com>> wrote: > > > On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby <ende...@apple.com > <mailto:ende...@apple.com>> wrote: >> On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator >> <revi...@reviews.llvm.org <mailto:revi...@reviews.llvm.org>> wrote: >> >> compnerd added a comment. >> >> Does anyone use the overload with the `O` for `exports` with `nullptr` >> instead of `this`? If not, we could just inline `this` throughout. > > This will break lld as it needs the default nullptr. See r308691. > > The reason that O was added was so that this check from r308690 could be > added. > > When O is specified is it always == this, though? That seems to be implied by > the original post. > > 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? > > > + if (O != nullptr) { > + if (State.Other > O->getLibraryCount()) { > + *E = malformedError("bad library ordinal: " + > Twine((int)State.Other) > + + " (max " + Twine((int)O->getLibraryCount()) + ") in export " > + "trie data at node: 0x" + utohexstr(offset)); > + moveToEnd(); > + return; > + } > > This is needed for the test case: > > +RUN: not llvm-objdump -macho -exports-trie > %p/Inputs/macho-trie-bad-library-ordinal 2>&1 | FileCheck -check-prefix > BAD_LIBRARY_ORDINAL %s > +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) > >> >> >> >> ================ >> Comment at: tools/llvm-nm/llvm-nm.cpp:1230 >> Error Err = Error::success(); >> - for (const llvm::object::ExportEntry &Entry : MachO->exports(Err, >> - MachO)) { >> + for (const llvm::object::ExportEntry &Entry : MachO->exports(Err)) { >> bool found = false; >> ---------------- >> I think that using `auto` here instead of `llvm::object:ExportEntry` is >> better for readability. >> >> >> ================ >> Comment at: tools/llvm-objdump/MachODump.cpp:9406 >> Error Err = Error::success(); >> - for (const llvm::object::ExportEntry &Entry : Obj->exports(Err, Obj)) { >> + for (const llvm::object::ExportEntry &Entry : Obj->exports(Err)) { >> uint64_t Flags = Entry.flags(); >> ---------------- >> Similar. >> >> >> Repository: >> rL LLVM >> >> https://reviews.llvm.org/D35961 <https://reviews.llvm.org/D35961> >> >> >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
