[PATCH] D44429: [Support][RFC] MessagePack reader/writer

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/MsgPackWriter.h:32
+
+#include "llvm/BinaryFormat/MsgPack.h"
+#include "llvm/Support/EndianStream.h"
----------------
This looks to be a layering violation - BinaryFormat depends on Support, not the other way around. So Support can't/shouldn't include BinaryFormat headers. (similar/same issue with the implementations in lib/Support)


================
Comment at: unittests/Support/MsgPackReaderTest.cpp:17-18
+
+class MsgPackReader : public ::testing::Test {
+public:
+  std::string Buffer;
----------------
This could be a struct if it only has public members and public inheritance


================
Comment at: unittests/Support/MsgPackReaderTest.cpp:22
+
+  MsgPackReader() {}
+};
----------------
Presumably this could be removed/implicit.


================
Comment at: unittests/Support/MsgPackReaderTest.cpp:43
+TEST_F(MsgPackReader, TestReadFixNegativeInt) {
+  for (int8_t i = -1; i >= -32; --i) {
+    Buffer.clear();
----------------
What makes these 32 values necessary/sufficient/interesting?


================
Comment at: unittests/Support/MsgPackReaderTest.cpp:54-78
+TEST_F(MsgPackReader, TestReadInt8) {
+  {
+    Buffer = std::string("\xd0\x00", 2);
+    Reader MPReader(Buffer);
+    EXPECT_EQ(MPReader.Read(Obj), true);
+    EXPECT_EQ(Obj.Kind, Type::Int);
+    EXPECT_EQ(Obj.Int, 0);
----------------
A test like this doesn't seem to benefit much from having 3 tests inside it (they don't share anything) - might as well split them into 3 separate TEST_Fs? (also this would give them distinct names that might help provide the motivation for each variation at a glance)

Similarly in the rest of these tests and in the Writer tests too.


================
Comment at: unittests/Support/MsgPackReaderTest.cpp:56
+  {
+    Buffer = std::string("\xd0\x00", 2);
+    Reader MPReader(Buffer);
----------------
std::string's 'assign' member might be more appropriate/direct


Repository:
  rL LLVM

https://reviews.llvm.org/D44429