----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25848/#review54879 -----------------------------------------------------------
Style review. One high-level comment: the additional fields for authoring doesn't seem to scale well - how about introducing a struct for that payload. In other words, let's try to limit the use of macros to a bare minimum. include/mesos/module.hpp.in <https://reviews.apache.org/r/25848/#comment95221> Can we use existing boost helpers for this? Something like http://www.boost.org/doc/libs/1_56_0/libs/preprocessor/doc/ref/cat.html include/mesos/module.hpp.in <https://reviews.apache.org/r/25848/#comment95222> You can wrap all the functions in extern "C" {} src/Makefile.am <https://reviews.apache.org/r/25848/#comment95193> Kill spaces - hard tabs in makefiles src/examples/test_module.hpp <https://reviews.apache.org/r/25848/#comment95194> { on new line src/examples/test_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95195> newline between includes from mesos, stout, ... src/examples/test_module_impl.cpp <https://reviews.apache.org/r/25848/#comment95196> Wrap according to http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ src/examples/test_module_impl2.cpp <https://reviews.apache.org/r/25848/#comment95197> Newline between the includes src/examples/test_module_impl2.cpp <https://reviews.apache.org/r/25848/#comment95198> Can we avoid complex types in global scope? And/or have const strings at least? src/examples/test_module_impl2.cpp <https://reviews.apache.org/r/25848/#comment95199> You can mark the whole block (all the MESOS_XXXX macros) as extern "C" to avoid duplication: extern "C" { ... } Opposed to extern "C" every time src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95224> Any particular reason for both using hashmap and maps? src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95209> const string&? Here and below src/module/manager.hpp <https://reviews.apache.org/r/25848/#comment95219> can we have const strings as keys? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95207> If we wanted to be pedantic from the start, how about setting all to MESOS_VERSION (like we discussed in the dev@ mailing thread) until we get comforatble with the api's stability? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95205> Mind indent according to http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ ? Bring down on newline and 4 space indent src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95206> Same here src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95204> Two newlines. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95203> Still a bit too dense - can we break up and comment on how it applies to the format defined in the header? src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95225> Two newlines. src/module/manager.cpp <https://reviews.apache.org/r/25848/#comment95217> can we have the key in libraryToModules be a const string instead? src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95215> Newline between the two src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95202> Two newlines between implementing functions src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95210> Can we have it return a const string? src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95201> bring this down on newline and indent 2 src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95211> See above - let's work a bit on constness of the strings here and below. src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95213> const string version? src/tests/module_tests.cpp <https://reviews.apache.org/r/25848/#comment95214> const strings? - Niklas Nielsen On Sept. 29, 2014, 2:16 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25848/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2014, 2:16 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Niklas Nielsen, > and Timothy St. Clair. > > > Bugs: MESOS-1384 > https://issues.apache.org/jira/browse/MESOS-1384 > > > Repository: mesos-git > > > Description > ------- > > Adding a first class primitive, abstraction and process for dynamic library > writing and loading can make it easier to extend inner workings of Mesos. > Making it possible to have dynamic loadable resource allocators, isolators, > containerizes, authenticators and much more. > > > Diffs > ----- > > configure.ac 86d448c3ad00ad01d3d069c1039dc7ad524af567 > include/mesos/module.hpp.in PRE-CREATION > src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b > src/examples/test_module.hpp PRE-CREATION > src/examples/test_module_impl.cpp PRE-CREATION > src/examples/test_module_impl2.cpp PRE-CREATION > src/module/manager.hpp PRE-CREATION > src/module/manager.cpp PRE-CREATION > src/tests/module_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/25848/diff/ > > > Testing > ------- > > Ran make check with added tests for verifying library/module loading and > simple version check. > > > Thanks, > > Kapil Arya > >
