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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/MsgPackWriter.h:32
+
+#include "llvm/BinaryFormat/MsgPack.h"
+#include "llvm/Support/EndianStream.h"
----------------
scott.linder wrote:
> 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.
No strong opinions on what the right layering is here - I guess if it's reasonable to have it up in BinaryFormat that's probably better than having it in Support. (usual "least scope" kind of approach - not always the right thing, but at least a vague guidance)


Repository:
  rL LLVM

https://reviews.llvm.org/D44429