[PATCH] D26477: Try to make ScopedPrinter use the new binary blob formatter

[Greg Clayton via llvm-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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



================
Comment at: include/llvm/Support/Format.h:206
 
-class FormattedHexBytes {
-  llvm::ArrayRef<uint8_t> Bytes;
-  // Display offsets for each line if FirstByteOffset has a value.
-  llvm::Optional<uint64_t> FirstByteOffset;
+class FormattedBinary {
+  ArrayRef<uint8_t> Bytes;
----------------
Binary sounds like a 0x12 hex byte would be displayed as 0b00010010. How about FormattedBytes?


================
Comment at: include/llvm/Support/Format.h:209
+
+  uint32_t IndentLevel; // Number of characters to indent each line.
+
----------------
This will align better if you move it after FirstByteOffset.


================
Comment at: include/llvm/Support/Format.h:220
 public:
-  FormattedHexBytes(llvm::ArrayRef<uint8_t> B, llvm::Optional<uint64_t> O,
-                    uint32_t NPL, uint8_t BGS, bool U, bool A)
-      : Bytes(B), FirstByteOffset(O), NumPerLine(NPL), ByteGroupSize(BGS),
-        Upper(U), ASCII(A) {}
+  FormattedBinary(ArrayRef<uint8_t> B, uint32_t IL, Optional<uint64_t> O,
+                  uint32_t NPL, uint8_t BGS, bool U, bool A)
----------------
FormattedBytes?


================
Comment at: include/llvm/Support/Format.h:230
 
-inline FormattedHexBytes format_hex_bytes(
-    llvm::ArrayRef<uint8_t> Bytes,
-    llvm::Optional<uint64_t> FirstByteOffset = llvm::Optional<uint64_t>(),
-    uint32_t NumPerLine = 16, uint8_t ByteGroupSize = 4) {
-  return FormattedHexBytes(Bytes, FirstByteOffset, NumPerLine, ByteGroupSize,
-                           false /*Upper*/, false /*ASCII*/);
+inline FormattedBinary format_binary(ArrayRef<uint8_t> Bytes,
+                                     uint32_t IndentLevel = 0,
----------------
format_bytes?


================
Comment at: include/llvm/Support/Format.h:231
+inline FormattedBinary format_binary(ArrayRef<uint8_t> Bytes,
+                                     uint32_t IndentLevel = 0,
+                                     Optional<uint64_t> FirstByteOffset = None,
----------------
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.


================
Comment at: include/llvm/Support/Format.h:241
+inline FormattedBinary
+format_binary_with_ascii(ArrayRef<uint8_t> Bytes, uint32_t IndentLevel = 0,
+                         Optional<uint64_t> FirstByteOffset = None,
----------------
format_bytes_with_ascii?


================
Comment at: include/llvm/Support/raw_ostream.h:26
 class FormattedNumber;
-class FormattedHexBytes;
+class FormattedBinary;
 template <typename T> class SmallVectorImpl;
----------------
FormatedBytes?


================
Comment at: include/llvm/Support/raw_ostream.h:227
+  // Formatted output, see the format_binary() function in Support/Format.h.
+  raw_ostream &operator<<(const FormattedBinary &);
 
----------------
FormatedBytes


================
Comment at: unittests/Support/raw_ostream_test.cpp:184
 
-std::string
-format_hex_bytes(const void *P, size_t N,
-                 llvm::Optional<uint64_t> Offset = llvm::Optional<uint64_t>(),
-                 uint32_t NumPerLine = 16, uint8_t ByteGroupSize = 4) {
+static std::string format_binary_str(ArrayRef<uint8_t> Bytes,
+                                     llvm::Optional<uint64_t> Offset = None,
----------------
format_bytes_str?


================
Comment at: unittests/Support/raw_ostream_test.cpp:195
 
-std::string format_hex_bytes_with_ascii(
-    const void *P, size_t N,
-    llvm::Optional<uint64_t> Offset = llvm::Optional<uint64_t>(),
+static std::string format_binary_with_ascii_str(
+    ArrayRef<uint8_t> Bytes, Optional<uint64_t> Offset = None,
----------------
format_bytes_with_ascii?


https://reviews.llvm.org/D26477