[PATCH] D48175: [BinaryFormat] Add MsgPackTypes

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked 2 inline comments as done.
scott.linder 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;
----------------
dblaikie wrote:
> 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.
The reason for this is that the methods to stream in MessagePack/YAML have to know which type to construct the document tree with. A single unique_ptr is implicitly convertible, but with a tree of them the conversions are not useful if you are trying to share parts of the tree. Unfortunately this does complicate the YAML trait specialization a bit.


================
Comment at: include/llvm/BinaryFormat/MsgPackTypes.h:50
+/// Abstract base-class which can be any MessagePack type.
+class Node {
+public:
----------------
dblaikie wrote:
> 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.
Thank you for the catch, fixed.


https://reviews.llvm.org/D48175