[PATCH] D44429: [BinaryFormat] MessagePack reader/writer

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Testing is verbose, but probably suitable given the API involved.

Layering - looks about right, though if this is only used by one place, it should just go there rather than in a more common library like this.



================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:17
+
+struct MsgPackWriter : public ::testing::Test {
+  std::string Buffer;
----------------
You can drop the 'public' here, since it's the default/assumed in a struct.


================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:17
+
+struct MsgPackWriter : public ::testing::Test {
+  std::string Buffer;
----------------
dblaikie wrote:
> You can drop the 'public' here, since it's the default/assumed in a struct.
LLVM doesn't generally use global scope operator (the leading '::' in '::testing::Test') unless needed - and I hope it's not needed here.


================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:548-549
+TEST_F(MsgPackWriter, TestWriteExt16Min) {
+  std::string s;
+  s.assign(static_cast<uint16_t>(UINT8_MAX) + 1, 'a');
+  MemoryBufferRef Ref = MemoryBufferRef(s, "");
----------------
You can probably collapse all code like this:

  std::string s;
  s.assign(num, char);

to this:

  std::string s(num, char);


================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:549
+  std::string s;
+  s.assign(static_cast<uint16_t>(UINT8_MAX) + 1, 'a');
+  MemoryBufferRef Ref = MemoryBufferRef(s, "");
----------------
What's the static_cast here for? I'd have thought the usual arithmetic conversions would've made UINT8_MAX + 1 work correctly here without the cast?


================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:552-553
+  MPWriter.writeExt(0x02, Ref);
+  s.insert(0, std::string("\xc8\x01\x00\x02", 4));
+  EXPECT_EQ(OStream.str(), s);
+}
----------------
Wonder if it makes more sense to change these two lines (& all similar lines in these tests) to:

  EXPECT_EQ(OStream.str(), std::string("...", 4) + s);


================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:577
+  s.assign(static_cast<uint32_t>(UINT16_MAX) + 1, 'a');
+  MemoryBufferRef Ref = MemoryBufferRef(s, "");
+  MPWriter.writeExt(0x02, Ref);
----------------
Probably write this as:

  MemoryBufferRef Ref(s, "");

Might actually even be better to not name this and roll it into its use below:

  MPWriter.writeExt(0x02, MemoryBufferRef(s, ""));

(similar feedback for other similar code in this patch/tests)


================
Comment at: unittests/BinaryFormat/MsgPackWriterTest.cpp:599-600
+#ifndef NDEBUG
+  MemoryBufferRef Ref;
+  Ref = MemoryBufferRef(s, "");
+  EXPECT_DEATH(CompatWriter.write(Ref), "compatible mode");
----------------
Looks slightly convoluted, compared to:

  MemoryBuffeRef Ref(s, "");

or is there a reason to write it the way it is rather than that ^ ?


https://reviews.llvm.org/D44429