Re: Review: YAML serialization
|
Jeremy Selan <jeremy...@...>
Nice work.
A few comments: OCIOYaml.cpp * There was a missing #include <cstring> * For the Enums, (line 512+) this could could be greatly simplified by relying on the helpers in OpenColorTypes.h. * You mention in the code, "GroupTransform seems to be a mixed concept". And, you dont allow for it to be serialized. Could you elaborate your thinking behind this? * The ExternalProject_Add stuff is slick, but appears to required cmake 2.8. We're currently only requiring 2.6. Is this update something we'd like to do? * On the Imageworks linux system, there is a build error related to libyaml-cpp: /usr/bin/ld: ../../ext/dist/lib/libyaml-cpp.a(node.cpp.o): relocation R_X86_64_32S against `std::basic_string<char, std::char_traits<char>, std::allocator<char> >::_Rep::_S_empty_rep_storage' can not be used when making a shared object; recompile with -fPIC I've addressed most of these in my local git repo in a low tech manner, by manually copying yaml into src/core/yaml. I've also patched yaml to put it into the OCIO namespace. see http://github.com/jeremyselan/OpenColorIO/tree/yaml While this approach isn't clean as your .tgz "ExternalProject_Add" solution, it's potentially less prone to build errors, and addresses all namespace issues. And, seeing as I imagine we wont be needing to update yaml versions on a regular basis, the maintenance implications aren't particularly troublesome either. Of course, I'm open to your approach of ExternalProject_Add assuming we actively agree to require cmake 2.8 (and address the build problems.) -- Jeremy On Sun, Oct 17, 2010 at 5:09 PM, Malcolm Humphreys <malcolmh...@mac.com> wrote: - Initial checkin of YAML serialization
|
|
|