mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"
Date Tue, 01 Dec 2015 13:08:40 GMT

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



src/slave/containerizer/fetcher.hpp (line 41)
<https://reviews.apache.org/r/40631/#comment167944>

    I agree with the intent to remove the using declaration out of `fetcher.hpp`, but I don't
agree with moving it into an internal namespace. Fetcher-related cpp files should pull it
in itself; `src/launcher/fetcher.cpp` for example already does so.
    
    I strongly disagree with introducing a using declaration to `src/tests/mesos.hpp`. It
doesn't seem to be something that should be common across all tests. It should be local to
fetcher-related tests; `src/tests/fetcher_tests.cpp` for example already pulls it in itself.
    
    My suggestion would be to use using declarations in cpp files, and use qualified names
in hpp files. We end up with the following changes:
    
    (1) Add `using mesos::fetcher::FetcherInfo;` in `src/slave/containerizer/fetcher.cpp`
and `src/tests/fetcher_cache_tests.cpp`.
    (2) `s/FetcherInfo/fetcher::FetcherInfo/` for 1 instance in `src/slave/containerizer/fetcher.hpp`
and 3 instances in `src/tests/mesos.cpp`.


- Michael Park


On Dec. 1, 2015, 12:51 p.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2015, 12:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3963
>     https://issues.apache.org/jira/browse/MESOS-3963
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> According to the google code style, the using should be used in internal namespace in
header files. Grep the header files, only fetcher.hpp deserved a path.
> 
> 
> > You may use a using-declaration anywhere in a .cc file (including in the global
namespace), and in functions, methods, classes, or within internal namespaces in .h files.
> 
> >Do not use using-declarations in .h files except in explicitly marked internal-only
namespaces, because anything imported into a namespace in a .h file becomes part of the public
API exported by that file.
> 
> ```
> // OK in .cc files.
> // Must be in a function, method, internal namespace, or
> // class in .h files.
> using ::foo::bar;
> ```
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 27971fe 
>   include/mesos/v1/mesos.proto 9acefd5 
>   src/common/resources.cpp 98804a4 
>   src/tests/resources_tests.cpp dbd39cd 
>   src/v1/resources.cpp 8488c31 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> -------
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>


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