----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21013/#review42328 -----------------------------------------------------------
Just a few small thoughts as it looks like Niklas has done a great job already! 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76127> These typically match the file name, any reason you didn't want to name the file dynamic_library.hpp? 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76093> Please put the C headers first and separated by a newline. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_and_Order_of_Includes#Names_and_Order_of_Includes. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76094> Why not use an Option for 'path_' instead of a sentinel value? 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76095> You don't need 'this->' and we typically omit it unless necessary. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76097> Do you want the library to be reopenable? seems like this is a resource leak. It's probably better to not allow this functionality and just return an error if 'path_' is Some: // Check if we've already opened a library. if (path.isSome()) { return Error("Library already opened"); } You could also close the existing library but I don't know what sort of ramifications that has on symbols that were loaded from that library (in terms of using variables or calling functions) so it's probably safer just to fail. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76099> You can kill this newline. 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment76100> So meta. ;-) - Benjamin Hindman On May 6, 2014, 7:05 p.m., Timothy St. Clair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21013/ > ----------------------------------------------------------- > > (Updated May 6, 2014, 7:05 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-1224 > https://issues.apache.org/jira/browse/MESOS-1224 > > > Repository: mesos-git > > > Description > ------- > > Addition of DynamicLibrary class to stout, to enable future plugins inside of > mesos. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/Makefile.am 980146b > 3rdparty/libprocess/3rdparty/stout/Makefile.am aa1d0a5 > 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp > PRE-CREATION > 3rdparty/libprocess/configure.ac 0c7cc6d > > Diff: https://reviews.apache.org/r/21013/diff/ > > > Testing > ------- > > New test has been added and ran 'make check' around Mac and Linux > > > Thanks, > > Timothy St. Clair > >
