[PATCH] D65255: [yaml2obj] Move core yaml2obj code into lib and include for use in unittests

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D65255#1602049 <https://reviews.llvm.org/D65255#1602049>, @MaskRay wrote:

> In D65255#1601122 <https://reviews.llvm.org/D65255#1601122>, @jhenderson wrote:
>
> > I suggested to Alex offline that he not try to do any more than the bare minimum to get this moved over. Certainly more work needs doing to it, but I think that can be done at a later point rather than upfront when moving it, given how useful it will be to have when working on the libObject code if nothing else.
>
>
> I agree. It'd be useful if @labath can elaborate which parts he intends to use in lldb. This will help decide how much of yaml2obj should be moved to lib/ObjectYAML.


I'd like have a function which takes a (yaml) string, and get raw bytes in return. So, that would be slightly less that what you're doing in the unit test here (as you're also re-parsing those bytes into an object file), but I am guessing you're going to need the re-parsing bits for the work you're doing anyway.

Also, due to how lldb's object parsers work, I'll need to save that stream of bytes into a file, but that is something that can be easily handled on the lldb side too.

As for object types, my main interest is ELF files, as we already have unit tests using those (by shelling out to yaml2obj). However, having COFF and MachO support would be nice too, and if it's available people might be inclined to use it. Minidump is interesting too, but I already have a mechanism for using that.

I don't know if that answers your question. If it doesn't, you'll have to ask me something more specific. :)



================
Comment at: llvm/lib/ObjectYAML/yaml2wasm.cpp:655
 
-int yaml2wasm(llvm::WasmYAML::Object &Doc, raw_ostream &Out) {
+int llvm::yaml::yaml2wasm(WasmYAML::Object &Doc, raw_ostream &Out) {
   WasmWriter Writer(Doc);
----------------
abrachet wrote:
> MaskRay wrote:
> > It is probably time to wrap these things in
> > 
> > ```
> > namespace llvm {
> > namespace yaml {
> > ...
> > }
> > }
> > ```
> I did this in all the files. Also I wrapped things in an anonymous namespace, is this ok?
llvm policy <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces> is to keep the anonymous namespaces as small as possible, preferring the `static` keyword instead. So, in this case, that would mean only the class definitions need to be wrapped in `namespace{}`.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:45
 
-static int convertYAML(yaml::Input &YIn, raw_ostream &Out) {
-  unsigned CurDocNum = 0;
----------------
abrachet wrote:
> MaskRay wrote:
> > There is probably not much we gain by moving this part to lib/ObjectYAML.
> I think its more convenient to not have to specify the file type. I wonder what others have to say.
I'm pretty ambivalent. On one hand, it's nice to have the same interface as the command line tool, but on the other, in the test you should always know the object type, so appending that to the function name does not hurt.


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

https://reviews.llvm.org/D65255