Re: Review: YAML serialization
Jeremy Selan <jeremy...@...>
A few comments:
* 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.
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
On Sun, Oct 17, 2010 at 5:09 PM, Malcolm Humphreys
- Initial checkin of YAML serialization