mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 45419: Cleaned up ModuleManager.
Date Tue, 29 Mar 2016 10:10:19 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45419/#review125849
-----------------------------------------------------------




src/module/manager.hpp (line 24)
<https://reviews.apache.org/r/45419/#comment188730>

    Not yours, but doesn't seem to be used.



src/module/manager.hpp (line 33)
<https://reviews.apache.org/r/45419/#comment188732>

    Not yours, but doesn't seem to be used.



src/module/manager.hpp (lines 34 - 36)
<https://reviews.apache.org/r/45419/#comment188731>

    Not yours, but could you update this with the complete include list? I see use of definitions
from at least `stout/errorbase.hpp`, `stout/foreach.hpp`, `stout/none.hpp`, `stout/option.hpp`
and `stout/try.hpp`.



src/module/manager.cpp (line 27)
<https://reviews.apache.org/r/45419/#comment188733>

    We don't seem to include headers both in the header and implementation file.



src/module/manager.cpp (lines 28 - 29)
<https://reviews.apache.org/r/45419/#comment188735>

    Not yours, but doesn't seem to be used.



src/module/manager.cpp (lines 31 - 32)
<https://reviews.apache.org/r/45419/#comment188724>

    Not yours, but could you sort these alphabetically?



src/module/manager.cpp (line 32)
<https://reviews.apache.org/r/45419/#comment188736>

    Not yours, but doesn't seem to be used.



src/module/manager.cpp (line 34)
<https://reviews.apache.org/r/45419/#comment188737>

    Could you makes these complete? I see the implementation is using definitions from at
least `stout/check.hpp`, `stout/nothing.hpp`, `stout/synchronized.hpp`, and `process/owned.hpp`.



src/module/manager.cpp 
<https://reviews.apache.org/r/45419/#comment188725>

    Usually we separate declarations outside of classes etc. by two empty lines, so I think
this line should not have been removed. Note that there is some inconsitency here, since strictly
following this rule would also make use set the declarations of above globals apart by two
empty lines.


- Benjamin Bannier


On March 29, 2016, 11:17 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45419/
> -----------------------------------------------------------
> 
> (Updated March 29, 2016, 11:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removes unused includes and some unsused usings from the ModuleManager.
> 
> 
> Diffs
> -----
> 
>   src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
>   src/module/manager.cpp 8c9aaf7cd00c904daba9994a99df9e1329831c01 
> 
> Diff: https://reviews.apache.org/r/45419/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message