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> wrote:
>
>
> On Fri, Jul 28, 2017 at 10:24 AM Kevin Enderby <ende...@apple.com> wrote:
>
>> On Jul 27, 2017, at 9:50 PM, Saleem Abdulrasool via Phabricator <
>> 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
>>
>>
>>
>>
>>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
