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

[Tim Renouf via Phabricator via llvm-commits]

tpr marked 6 inline comments as done.
tpr 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());
+          });
----------------
scott.linder wrote:
> tpr wrote:
> > foad wrote:
> > > 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?
> > It's a bit of a corner case, but, in the code before, I think you could have a map containing two entries of the same name, both maps, and it would merge them together. Having two entries of the same name is probably not legal MsgPack in some sense.
> Isn't the behavior already changing? In the old code any existing document is discarded and you essentially parse a fresh document to replace it. In the new code the default is effectively inverted, and if you don't do anything special in your `Merger` you get the existing document with only distinct bits added in from the newly parsed one. If we are completely changing the semantics anyway it seems reasonable to fix odd bits like this.
> 
> Alternatively we could just have Merger be `Optional` and retain the behavior completely if it is not present.
OK, I've gone with Jay's suggestion of the default being to fail on any conflict.


================
Comment at: llvm/lib/BinaryFormat/MsgPackDocument.cpp:142
       auto &Array = Stack.back().Node.getArray();
-      Array.push_back(Node);
+      DestNode = &Array[Stack.back().Count++];
     } else {
----------------
dineshkb-amd wrote:
> 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?
Modified to append when merging arrays.


================
Comment at: llvm/lib/BinaryFormat/MsgPackDocument.cpp:158
     }
+    if (!DestNode->isNil()) {
+      // In a merge, there is already a value at this position. Call the
----------------
scott.linder wrote:
> Shouldn't this check for `Empty`? Why is `Nil` special concerning merges?
> 
> Is this because we represent an empty document as a `Nil` node? I didn't notice that in the original review, but I think we need to represent an empty document in a way that is distinct from one of the possible documents (in this case we can't distinguish an empty document from a document containing a single `Nil` node).
Good point. I have now separated the idea of an empty node from a nil one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79671