[PATCH] D48175: [BinaryFormat] Add MsgPackTypes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/BinaryFormat/MsgPackTypes.h:32-35
+/// Template to choose between a std::unique_ptr and std::shared_ptr
+template <class T, bool U>
+using Ptr =
+    typename std::conditional<U, std::unique_ptr<T>, std::shared_ptr<T>>::type;
----------------
What's the need for this?

I suspect it's probably best to have the APIs always return unique_ptrs - which are implicitly convertible to shared_ptrs if the users of the API need that.


================
Comment at: include/llvm/BinaryFormat/MsgPackTypes.h:50
+/// Abstract base-class which can be any MessagePack type.
+class Node {
+public:
----------------
Looks like this probably needs a virtual dtor? Unless there's something to prevent polymorphic ownership (ie: to prevent someone taking, say, a unique_ptr<ArrayNode> to a unique_ptr<Node>) - such as having a protected dtor.


================
Comment at: unittests/BinaryFormat/MsgPackTypesTest.cpp:133
+  yaml::Output yout(OStream);
+  auto M = makePtr<MapNode<>>();
+  (*M)["foo"] = (makePtr<ScalarNode>(int64_t(1)));
----------------
Looks like this could be a direct value rather than dynamically allocated:

  MapNode<> M;


================
Comment at: unittests/BinaryFormat/MsgPackTypesTest.cpp:134-137
+  (*M)["foo"] = (makePtr<ScalarNode>(int64_t(1)));
+  (*M)["bar"] = (makePtr<ScalarNode>(uint64_t(2)));
+  auto N = makePtr<MapNode<>>();
+  (*N)["baz"] = (makePtr<ScalarNode>(true));
----------------
Drop extra parens around these RHS, so it's like this:

  ... = makePtr<...>(...);


https://reviews.llvm.org/D48175