This is an archive of the discontinued LLVM Phabricator instance.

Try to make ScopedPrinter use the new binary blob formatter
ClosedPublic

Authored by zturner on Nov 9 2016, 3:08 PM.

Details

Summary

ScopedPrinter was already using its own algorithm to format binary blob data. With the new method added by clayborg, I wanted to get down to a single implementation.

In the process of doing this, I needed to update the code a little bit to support some things that ScopedPrinter needed to do. Here are the material changes from the previous implementation:

  1. Changed name from format_hex_bytes to format_binary. I felt format_hex_bytes was too similar to format_hex.
  2. Added the ability to offset each line of the binary blob by an initial offset.
  3. Fixed a bug in the implementation where we were calling the equivalent of isprint((char)Byte), which can cause the argument to isprint() to be negative, which is undefined behavior.
  4. Got a little smarter about how wide to print the offset field. Previously we were just using a full 8 bytes for the offset field, but with this patch I calculate the maximum offset value we will need to print, determine how many nibbles are needed to print that value, and then align all offset values to that width.
  5. Remove the 0x prefix from the offset. I could have added an option to choose whether we want the prefix, and if anyone really needs that I can add it back. I removed it because already have a lot of tests that will fail if the offset is there, and keeping the API simple seems desirable if nobody really needs that functionality.
  6. Made a few things more idiomatic / LLVMish. For example, None instead of a default constructed Optional<uint64_t>(), remove null checking from unit test helper functions (pointless since we control all entry points into the function), changing function signatures to accept ArrayRef instead of const void * and length.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 77400.Nov 9 2016, 3:08 PM
zturner retitled this revision from to Try to make ScopedPrinter use the new binary blob formatter.
zturner updated this object.
zturner added reviewers: mehdi_amini, clayborg.
zturner added a subscriber: llvm-commits.
clayborg requested changes to this revision.Nov 10 2016, 9:09 AM
clayborg edited edge metadata.

See inlined comments. Mostly renaming, layout and argument order stuff.

include/llvm/Support/Format.h
206 ↗(On Diff #77400)

Binary sounds like a 0x12 hex byte would be displayed as 0b00010010. How about FormattedBytes?

209 ↗(On Diff #77400)

This will align better if you move it after FirstByteOffset.

220 ↗(On Diff #77400)

FormattedBytes?

230 ↗(On Diff #77400)

format_bytes?

231 ↗(On Diff #77400)

I would vote to move this argument after the FirstByteOffset and possibly past NumPerLine and ByteGroupSize? I would think indent level isn't going to be changed to non-zero many by that many clients.

241 ↗(On Diff #77400)

format_bytes_with_ascii?

include/llvm/Support/raw_ostream.h
26 ↗(On Diff #77400)

FormatedBytes?

227 ↗(On Diff #77400)

FormatedBytes

unittests/Support/raw_ostream_test.cpp
184 ↗(On Diff #77400)

format_bytes_str?

195 ↗(On Diff #77400)

format_bytes_with_ascii?

This revision now requires changes to proceed.Nov 10 2016, 9:09 AM
zturner updated this revision to Diff 77508.Nov 10 2016, 10:33 AM
zturner edited edge metadata.

Updated with Clayborg's suggestions

clayborg accepted this revision.Nov 10 2016, 11:12 AM
clayborg edited edge metadata.

Looks good.

lib/Support/raw_ostream.cpp
355 ↗(On Diff #77508)

One thing that I was wondering is can we add an operator to the FormattedBytes class:

class FormattedBytes {
public:
    raw_ostream &operator<<(raw_ostream &s);
};

And avoid this code being in raw_ostream.cpp? Seems wrong to put the implementation for dumping FormattedBytes into raw_ostream.cpp. Doesn't need to be done with this patch, but I found this not to be very C++.

This revision is now accepted and ready to land.Nov 10 2016, 11:12 AM
zturner added inline comments.Nov 10 2016, 11:20 AM
lib/Support/raw_ostream.cpp
355 ↗(On Diff #77508)

You might be interested in D25587. My long term goal is to move away from all these overloaded operators entirely and be able to use printf-style formatting for everything. You can imagine writing OS << formatv("Bytes (Length={0}): ({1})", Data.size(), fmt_bytes(Data)), for example.

When that happens, we'll be able to isolate the formatting code to more appropriate places.

This revision was automatically updated to reflect the committed changes.