Page MenuHomePhabricator

[llvm-dwarfdump] --statistics: switch to json::OStream. NFC
ClosedPublic

Authored by MaskRay on Aug 16 2020, 6:26 PM.

Details

Summary

Then it is trivial to make the output indented (the second parameter of
json::OStream::OStream specifies the indentation).

Diff Detail

Event Timeline

MaskRay created this revision.Aug 16 2020, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2020, 6:26 PM
MaskRay requested review of this revision.Aug 16 2020, 6:26 PM
MaskRay added inline comments.Aug 16 2020, 6:28 PM
llvm/tools/llvm-dwarfdump/Statistics.cpp
482

I am not sure repeating the message in LLVM_DEBUG is useful.

I can delete them in a separate change if people think the same as me.

jhenderson accepted this revision.Aug 18 2020, 2:17 AM
jhenderson added subscribers: Orlando, dblaikie.

Since this is JSON we're trying to emit, using the JSON stream makes sense to me. LGTM, but might be worth hanging on a bit to give others a chance to chime in.

llvm/tools/llvm-dwarfdump/Statistics.cpp
482

Not sure I've got any reasonable opinion here, since I don't really use this code or LLVM_DEBUG output ever. I'd prefer of one of the other typical users chime in here (e.g. @aprantl, @dblaikie, @Orlando)

This revision is now accepted and ready to land.Aug 18 2020, 2:17 AM
Orlando added inline comments.Aug 18 2020, 2:36 AM
llvm/tools/llvm-dwarfdump/Statistics.cpp
482

I'm not sure how much weight my opinion carries here but I agree with removing the prints. I have never used these particular debug prints. When I want to inspect the output I usually just pipe it through python -m json.tool to format it.

aprantl added inline comments.Aug 18 2020, 4:02 PM
llvm/tools/llvm-dwarfdump/Statistics.cpp
482

I am not sure repeating the message in LLVM_DEBUG is useful.

The original idea was to have human-readable output on stderr and JSON output on stdout. But if this is no longer useful, I'm fine with removing it.

MaskRay added inline comments.Aug 18 2020, 4:09 PM
llvm/tools/llvm-dwarfdump/Statistics.cpp
482

With

- json::OStream J(OS);
+ json::OStream J(OS, 2);

the JSON will be broken into multiple lines with an indentation of 2. I think that is quite readable, no need for stderr output.

Can I get more approval power for this NFC change?

echristo accepted this revision.Aug 19 2020, 2:48 PM
echristo added a subscriber: echristo.

I feel pretty OK approving this.

(Tradeoffs I'd contemplated and discarded was renaming OS rather than OS->J everywhere. I ended up liking the readability of J more. JS might be nice as well?)

-eric

I feel pretty OK approving this.

(Tradeoffs I'd contemplated and discarded was renaming OS rather than OS->J everywhere. I ended up liking the readability of J more. JS might be nice as well?)

-eric

Do you mean removing the OS->J rename? That would be a bit ambiguous because people usually use OS to refer to raw_ostream and its derivatives. The JSON stream is special.

I feel pretty OK approving this.

(Tradeoffs I'd contemplated and discarded was renaming OS rather than OS->J everywhere. I ended up liking the readability of J more. JS might be nice as well?)

-eric

Do you mean removing the OS->J rename? That would be a bit ambiguous because people usually use OS to refer to raw_ostream and its derivatives. The JSON stream is special.

Yep. And that's one of the reasons why I discarded that one. JS would probably make sense (JSON Stream) if you'd like. No strong opinions.

-eric

I feel pretty OK approving this.

(Tradeoffs I'd contemplated and discarded was renaming OS rather than OS->J everywhere. I ended up liking the readability of J more. JS might be nice as well?)

-eric

Do you mean removing the OS->J rename? That would be a bit ambiguous because people usually use OS to refer to raw_ostream and its derivatives. The JSON stream is special.

Yep. And that's one of the reasons why I discarded that one. JS would probably make sense (JSON Stream) if you'd like. No strong opinions.

-eric

Thanks! I misread the previous comment (keeping the "OS" name was contemplated but discarded). I'll commit this as is tomorrow if nobody expresses a preference.