----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44694/#review123083 -----------------------------------------------------------
Fix it, then Ship it! src/module/manager.cpp (line 22) <https://reviews.apache.org/r/44694/#comment185214> we don't need this right? src/module/manager.cpp (lines 207 - 208) <https://reviews.apache.org/r/44694/#comment185215> Can we clarify this comment to say that we expect the parameter lists to match exactly? (i.e. same order) src/module/manager.cpp (line 210) <https://reviews.apache.org/r/44694/#comment185217> You can safely capture by `const&` src/module/manager.cpp (lines 212 - 229) <https://reviews.apache.org/r/44694/#comment185216> I think you can re-organize this not to have the error message twice. bool parameterError = module.parameters().size() != parameters.parameter().size(); if (!parameterError) { ... // set parameterError if not matching. } if (parameterError) { return Error(); } src/module/manager.cpp (line 214) <https://reviews.apache.org/r/44694/#comment185221> need a whitespace or it will concat `samename`? src/module/manager.cpp (lines 219 - 220) <https://reviews.apache.org/r/44694/#comment185218> You can safely capture by `const&`. How about `lhs` and `rhs`? src/module/manager.cpp (lines 221 - 222) <https://reviews.apache.org/r/44694/#comment185219> Do you mean `||` instead of `&&`? src/module/manager.cpp (line 224) <https://reviews.apache.org/r/44694/#comment185222> same comment as above (in case you keep 2 copies of the message) src/module/manager.cpp (line 244) <https://reviews.apache.org/r/44694/#comment185220> do you need a whitespace at the end? - Joris Van Remoortere On March 11, 2016, 2:51 a.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44694/ > ----------------------------------------------------------- > > (Updated March 11, 2016, 2:51 a.m.) > > > Review request for mesos, Joris Van Remoortere, Michael Park, and Till > Toenshoff. > > > Bugs: MESOS-4903 > https://issues.apache.org/jira/browse/MESOS-4903 > > > Repository: mesos > > > Description > ------- > > As long as the module manifest(s) being loaded don't conflict with the > already loaded manifests, the module manager will allow multiple calls > to `load()`. > > > Diffs > ----- > > src/module/manager.hpp 89dd06097a355c6455e8f8af7d3d98847a304c90 > src/module/manager.cpp 6ae99504005581b22a44768949b1d305cec517d9 > src/tests/module_tests.cpp 497ed87bddce23bb457373da40dc87111c04136d > > Diff: https://reviews.apache.org/r/44694/diff/ > > > Testing > ------- > > Added new tests and ran make check. > > > Thanks, > > Kapil Arya > >