[PATCH] D44429: [Support][RFC] MessagePack reader/writer

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In https://reviews.llvm.org/D44429#1048571, @chandlerc wrote:

> I haven't really had time to look at this, but it seems somewhat unfortunate to add yet another format like this. =[ I guess I'm surprised that AMD is looking at baking this into their object file format. I'm not sure it makes sense to really consider landing this until the usage of it is at least available for review as well. While having the patches separate is good, I think it would be important to see the actual planned usage before adding all this code.
>
> @dblaikie may have comments about the layering and the unittesting.


Thank you for taking a look and adding subscribers; I didn't know who might have interest.

I understand the aversion to merging unused code, I just wanted to get feedback on the possibility as soon as possible. As for supporting another format I also understand the reluctance, but the MessagePack specification seems to be fairly static, and the code implementing it is relatively concise and straightforward. I do not think this code will have a significant impact on maintenance.

I am beginning work now on code for the AMD object file which will use MessagePack, so I should have more context available for this patch soon.



================
Comment at: include/llvm/Support/MsgPackWriter.h:32
+
+#include "llvm/BinaryFormat/MsgPack.h"
+#include "llvm/Support/EndianStream.h"
----------------
dblaikie wrote:
> This looks to be a layering violation - BinaryFormat depends on Support, not the other way around. So Support can't/shouldn't include BinaryFormat headers. (similar/same issue with the implementations in lib/Support)
Do you have thoughts on the best approach to fixing the layering? BinaryFormat seems like the correct place for the MessagePack constants, but I don't see why it can't be moved. Similarly, Support seems like the correct place for the MessagePack (de)serializer, but perhaps there is somewhere else it could live, possibly in BinaryFormat.

I really appreciate you taking a look, so thank you. I will address what I can and think of how best to move things to preserve layering.


Repository:
  rL LLVM

https://reviews.llvm.org/D44429