mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Klaus Ma" <klaus1982...@gmail.com>
Subject Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"
Date Tue, 01 Dec 2015 12:16:36 GMT


> On Nov. 24, 2015, 5:42 p.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > <https://reviews.apache.org/r/40631/diff/1/?file=1138326#file1138326line87>
> >
> >     This seems like a weird addition for this patch: while the existing `using`
decls above could be justified (it's hard to imagine testing test infrastructure w/o having
to perform some environment cleanup), unconditionally pulling in `mesos::fetcher::FetcherInfo`
here seems way too general; also, it probably would make any attempt of e.g., mocking `FetcherInfo`
anywhere much harder.
> >     
> >     Either pull this into the internal namespace, or just do the extra typing here
and pull it in in the corresponding `mesos.cpp`.
> 
> Klaus Ma wrote:
>     @Guangya/Ben, I'm thinking mesos.hpp as internal header :). Anyweay, maybe we move
`using mesos::internal::slave::FetherInfo` into `mesos::internal::test` in `mesos.hpp`.
> 
> Benjamin Bannier wrote:
>     I think the guideline here is to limit the scope of using declarations as much as
possible, so moving it to the smallest possible scope (which here unfortunately is still large)
is preferred.
> 
> Michael Park wrote:
>     Sorry, I still don't understand why this was introduced to begin with. Could you
explain?
> 
> Benjamin Bannier wrote:
>     A using decl was dropped in `src/slave/containerizer/fetcher.hpp`, and since we are
often confused by long names (even though we like namespaces) we need to add this here for
a function decl below.

@mcypark, according to our code style document, we should not use global `using` in header
file. So I start this RR to move `mesos::fetcher::FetcherInfo` into an internal namespace.


- Klaus


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


On Nov. 25, 2015, 9:54 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2015, 9:54 a.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
> -----
> 
>   src/slave/containerizer/fetcher.hpp 78e7d14 
>   src/tests/mesos.hpp a2a76f5 
> 
> 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