[PATCH] D57023: [MsgPack] New MsgPackDocument class

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Probably best to have someone else from AMDGPU land review this for your uses there - I'm just giving some high level at-a-glance stylistic coverage here.



================
Comment at: include/llvm/BinaryFormat/MsgPackDocument.h:237
+    for (auto I : Strings)
+      delete[] I;
+    Strings.clear();
----------------
Any chance of using unique_ptrs in this data structure so manual cleanup isn't required?

Or change it to a vector of std::string?


================
Comment at: lib/BinaryFormat/MsgPackDocument.cpp:30
+/// DocNode comparison operator, used for map keys.
+bool DocNode::operator<(const DocNode &Other) const {
+  // This has to cope with one or both of the nodes being default-constructed,
----------------
Prefer non-member overloads when possible - it ensures equal conversion handling for the left and right operands. (the operator can still be implemented as a friend, so using all the implementation details, etc)


================
Comment at: lib/BinaryFormat/MsgPackDocument.cpp:73-77
+  auto It = find(Key);
+  if (It != end())
+    return It->second;
+  // Ensure a new element has its KindAndDoc initialized.
+  auto &N = (*Map)[Key];
----------------
Could you use insert (& check the bool in the pair returned from insert to determine if it was newly inserted, then do the expensive (getDocument()->getNode()) initialization only in that case) instead of find+lookup? That way the code doesn't do two map lookups in the new case, only one.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57023/new/

https://reviews.llvm.org/D57023