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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D65255#1600507 <https://reviews.llvm.org/D65255#1600507>, @labath wrote:

> Thanks for taking this on. I look forward to being able to use this in lldb tests.
>
> I'm not an owner here, but the main question I have is about the library-readiness of the code you're moving. I see it's doing things like spewing errors to stderr and even calling exit(), neither of which is a very nice thing to do for a library (even if it's just a "test" library). Do you have any plans for addressing that?


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.



================
Comment at: llvm/include/llvm/ObjectYAML/yaml2obj.h:52
+
+int convertYAML(Input &YIn, raw_ostream &Out, unsigned DocNum = 1);
+
----------------
Let's have this return an llvm::Error. I shouldn't massively increase the complexity of this code.


================
Comment at: llvm/lib/ObjectYAML/CMakeLists.txt:1
 add_llvm_library(LLVMObjectYAML
   CodeViewYAMLDebugSections.cpp
----------------
Looks like this CMake is missing any reference to the include files, which means they won't get auto-magically added to things like Visual Studio's projects. If you take a look at libObject's CMakeLists.txt, you'll find an example of what to do.


================
Comment at: llvm/lib/ObjectYAML/CMakeLists.txt:17-22
+  yaml2coff.cpp
+  yaml2elf.cpp
+  yaml2macho.cpp
+  yaml2obj.cpp
+  yaml2minidump.cpp
+  yaml2wasm.cpp
----------------
My instinct is that we should rename these files to match the naming style of other files in this library. Open to other suggestions though.


================
Comment at: llvm/lib/ObjectYAML/yaml2obj.cpp:19-22
+LLVM_ATTRIBUTE_NORETURN static void error(Twine Message) {
+  errs() << Message << "\n";
+  exit(1);
+}
----------------
We should avoid functions like this in the new library code. Better would be something that returned llvm::Error. In practice, you should probably just call createStringError at all its call sites and pass the resultant Error up the stack.


================
Comment at: llvm/unittests/ObjectYAML/CMakeLists.txt:9
   YAMLTest.cpp
+  yaml2ObjectFileTest.cpp
   )
----------------
Looks like filenames usually start with an upper-case letter. Also, the filenames usually reflect the names of the file they are testing, in this case yaml2obj.cpp, so let's call this YAML2ObjTest.cpp.


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

https://reviews.llvm.org/D65255