----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21013/#review42210 -----------------------------------------------------------
Hey Tim, Feel free to reach out on IRC or on mail if you disagree on the points I raised. I have a a version here: https://github.com/nqn/mesos/commit/abcc07ac74d3c1a7d0a854f00bc949088bdec689 with the changes I suggest. Let me know what you think. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75963> Again, it's better to do this in an initializer list: DynamicLibrary() : handle(NULL), librarypath("NOTLOADED") { } 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75998> 4 spaces should be 2. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75991> We try to keep the member variables clean of "_" and in case of clashes, do that locally. So in this case "librarypath_" as argument. s/Try<bool>/Try<Nothing>/ here and in the other methods. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75982> The indentation seems wrong here. Should be 4 spaces: return Error( "Could not load library '" + librarypath + "': " + dlerror()); 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75961> Trailing space. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75995> No need for else clause after return in if block. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76004> return Nothing(); 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75983> s/Try<bool>/Try<Nothing>/ here and in the other methods. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75987> Indentation 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75962> Trailing space. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75997> Same - no need for else clause. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76005> return Nothing(); 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75967> Why psymbol? Is it an abbreviation for something? If so, let's expand or drop the 'p' Also, instead of a return argument. How about using Try<void*> instead and return it? symbolname should be camel case too right? Lastly, "sym" is an abbreviation. should it be "symbol" or "loadSymbol" ? 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75984> The seems odd here too: return Error( "Error looking up symbol '" ... librarypath_ + "' : " ... 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment76006> return Nothing(); 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75965> See comment above. Let's try to strip '_'. 3rdparty/libprocess/3rdparty/stout/include/stout/dynamiclibrary.hpp <https://reviews.apache.org/r/21013/#comment75992> librarypath should be camel case too? 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment75986> TEST is not a very descriptive test name. Also, how about having negative tests? Make sure that when we try to resolve unknown symbols that we get an error? So this test name could be "DynamicLibraryTest, loadKnownSymbol" And we could have: "DynamicLibraryTest, loadUnknownLibrary" and/or "DynamicLibraryTest, loadUnknownSymbol"? 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment75980> s/checkresult/result/ 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment76007> s/checkresult/result/g 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment75966> Would #ifdef __linux__ work? It is used in other tests as far as I know. 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment75985> EXPECT_TRUE(result.isSome()); will give a clean message if the call actually fails (and will be enough if you change Try<bool> to Try<Nothing> 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment75988> You used psymbol (lower case) before. Can you use "symbol" instead? 3rdparty/libprocess/3rdparty/stout/tests/dynamiclibrary_tests.cpp <https://reviews.apache.org/r/21013/#comment75981> s/sym (/sym(/ With the Try<void*> change: Try<void*> symbol = dltest.sym("dlopen"); ? - Niklas Nielsen On May 5, 2014, 8:22 a.m., Timothy St. Clair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21013/ > ----------------------------------------------------------- > > (Updated May 5, 2014, 8:22 a.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 > >
