[PATCH] D41657: Do not look up symbol names when n_strx == 0

Davide Italiano via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 2 14:52:20 PST 2018


davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D41657#966001, @mtrent wrote:

> In https://reviews.llvm.org/D41657#965799, @davide wrote:
>
> > I wonder whether you can use something like `yaml2obj` to craft the object? (or, assuming it's valid, `llvm-mc`)?
> >  That would improve the readability a lot IMHO.
> >  If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?
>
>
> llvm-objdump and llvm-nm commonly use binary tests when working with bad binaries, so this test isn't unusual. It's not clear to me how you would regenerate this mach-o with lld.


It happened in the past that changes break tests. If you have a binary checked in, it's harder to understand what was the original intent, IMHO.
That's why I was recommending to use YAML, if possible at all. That said, I don't think it's critical, and your comment is explicative enough.

This LGTM after the two minors are fixed.


https://reviews.llvm.org/D41657





More information about the llvm-commits mailing list