This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Keep warning for --disassemble-functions in correct order.
ClosedPublic

Authored by ychen on Jul 3 2019, 4:09 PM.

Event Timeline

ychen created this revision.Jul 3 2019, 4:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 4:09 PM

LG. A few nits below:

llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test
6
yaml2obj -o %t.o %s
rm -f %t.a
llvm-ar qc %t.a %t.o %t.o
llvm/tools/llvm-objdump/llvm-objdump.cpp
388–389

Why was StringRef Prefix added? It does not seem to be used.

393

This is probably redundant. stderr is by default unbuffered. If the user makes it buffered, they should be prepared to see out-of-order output.

jhenderson accepted this revision.Jul 4 2019, 1:52 AM

LGTM, with MaskRay's comments addressed.

This revision is now accepted and ready to land.Jul 4 2019, 1:52 AM
ychen updated this revision to Diff 208433.Jul 8 2019, 8:38 AM
  • update
ychen marked 4 inline comments as done.Jul 8 2019, 8:44 AM
ychen added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
388–389

It was intended for the patch for PR35351. I've removed it.

393

Agree that it seems redundant but it makes explicit that stderr should be flushed there. Three other places in the same file use it too. I could remove them all or keep them all otherwise it is really confusing and inconsistent. Thoughts?

ychen edited the summary of this revision. (Show Details)Jul 8 2019, 12:37 PM
ychen marked 2 inline comments as done.
ychen added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
388–389

I meant to say PR41911.

MaskRay accepted this revision.Jul 8 2019, 9:50 PM
This revision was automatically updated to reflect the committed changes.