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.

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #207910)
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
384 ↗(On Diff #207910)

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

389 ↗(On Diff #207910)

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
384 ↗(On Diff #207910)

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

389 ↗(On Diff #207910)

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
384 ↗(On Diff #207910)

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.