[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.
More information about the llvm-commits