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

[Dineshkumar Bhaskaran via Phabricator via llvm-commits]

dineshkb-amd added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/MsgPackDocument.h:371
+            return (DestNode->isMap() && SrcNode.isMap()) ||
+                   (DestNode->isArray() && SrcNode.isArray());
+          });
----------------
Can we consider *DestNode = SrcNode if DestNode and SrcNode are of same type to be closer to old behavior.


================
Comment at: llvm/lib/BinaryFormat/MsgPackDocument.cpp:142
       auto &Array = Stack.back().Node.getArray();
-      Array.push_back(Node);
+      DestNode = &Array[Stack.back().Count++];
     } else {
----------------
One of the logical cases to consider for mergers involving ArrayNodes is that the "Merger" callback provider can append Node elements to DestNode. e.g. lets say the document looking like 

Root{"K1" : [ {"P" : p1} ] } is merged with Root{"K1" : ["P" : p2} ] } where {} represents a map and [] represents an array. The expected result is Root{"K1" : [ {"P" : p1}, {"P" : p2} ] } 

This is one of the cases when we deal with merging metadata in ROCm CompilerSupport. Can this be achieved with current callback if so shouldn't the Array index be modified to the number of elements added?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79671