[PATCH] D127776: [ObjectYAML] Add offloading binary implementations for obj2yaml and yaml2obj

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/OffloadYAML.h:31-32
+  struct Member {
+    Optional<StringRef> ImageKind;
+    Optional<StringRef> OffloadKind;
+    Optional<uint32_t> Flags;
----------------
I'd rather expect these two to be enums that correspond to the IMG and OFK values in the spec, with the option of specifying an arbitrary uint16_t value so that you can provide arbitrary values that don't have a defined mapping (this is a common pattern in ELF yaml2obj, for example).


================
Comment at: llvm/include/llvm/ObjectYAML/OffloadYAML.h:38
+
+  std::vector<Member> Members;
+};
----------------
The spec discsusses a wider range of fields than just the members themselves. In particular, I'd expect to see an optional Version field, so that you can test using an arbitrary version number. Having optional size and offset fields would also allow you to override the defaults of these values, allowing you to test error paths better (e.g. to show that if the offset is past the end of the file, an error message is reported). The one field you probably don't need is the `Magic` field, in my opinion.


================
Comment at: llvm/test/ObjectYAML/Offload/default.yaml:12
+# CHECK-NEXT: ...
+
----------------
Nit: looks like you've got multiple blank lines at EOF. The file should end in exactly one '\n', no more, no less.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127776