[PATCH] D124945: [ObjectYAML][DX] Add dxcontainer2yaml support

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/tools/obj2yaml/dxcontainer2yaml.cpp:19
+
+static Expected<DXContainerYAML::Object *>
+dumpDXContainer(MemoryBufferRef Source) {
----------------
Returning a raw pointer? Is this a yaml-ism?


================
Comment at: llvm/tools/obj2yaml/dxcontainer2yaml.cpp:21
+dumpDXContainer(MemoryBufferRef Source) {
+  assert(file_magic::dxcontainer_object == identify_magic(Source.getBuffer()));
+
----------------
Looks like this is accessible via `dxcontainer2yaml` without a runtime check on the magic? For parsing API surface like this I would usually go with an `llvm::Error` rather than an assert, since I don't trust API clients to verify the magic.

In this case you're unlikely to have any clients outside `obj2yaml` though, so this is mostly pedantry on my part. Go with whichever approach you prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124945