[PATCH] D79671: [MsgPack] MsgPackDocument::readFromBlob now merges

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/MsgPackDocument.h:370-371
+          Merger = [](DocNode *DestNode, DocNode SrcNode, DocNode MapKey) {
+            return (DestNode->isMap() && SrcNode.isMap()) ||
+                   (DestNode->isArray() && SrcNode.isArray());
+          });
----------------
tpr wrote:
> foad wrote:
> > Can't the body of this default merger be just `llvm_unreachable();`?
> Probably, although that is a change in behavior from before, which I'd rather not do as I think I am not the only user of it. (Also, even if we did do that, it should be "return false" so that the readFromBlob call returns false for failure.)
I don't get it. Surely in existing use cases, DestNode is nil so the merger will not be called. And if it *was* called, there's no point in it returning true unless it actually copies some data from SrcNode to DestNode, otherwise it'll claim to have done a merge but actually it ignored the new incoming data and left the document exactly as it was (i.e. empty). What am I missing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79671