Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

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

(Updated Dec. 4, 2015, 3:11 p.m.)


Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.


Changes
---

Move `using` into .cpp files


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 (updated)
-

  src/slave/containerizer/fetcher.hpp 78e7d14 
  src/slave/containerizer/fetcher.cpp 26df3d5 
  src/tests/fetcher_cache_tests.cpp fb0b3ba 
  src/tests/mesos.hpp 8d2d919 
  src/tests/mesos.cpp d42dab5 

Diff: https://reviews.apache.org/r/40631/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40631]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 1:13 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, 1:13 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
> -
> 
>   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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

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

(Updated Dec. 1, 2015, 8:51 p.m.)


Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.


Changes
---

Add comments for `RevocableInfo::Type`


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 (updated)
-

  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

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Michael Park]

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



src/slave/containerizer/fetcher.hpp (line 41)


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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

> On Nov. 24, 2015, 5:42 p.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > 
> >
> > 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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

> On Dec. 1, 2015, 9:08 p.m., Michael Park wrote:
> > src/slave/containerizer/fetcher.hpp, line 43
> > 
> >
> > 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`.

Agree :). I'll update them accordingly.


- Klaus


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


On Dec. 1, 2015, 9:13 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, 9:13 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
> -
> 
>   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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Benjamin Bannier]

> On Nov. 24, 2015, 9:42 a.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > 
> >
> > 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?

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.


- Benjamin


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


On Nov. 25, 2015, 1: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, 1: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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Michael Park]

> On Nov. 24, 2015, 9:42 a.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > 
> >
> > 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.

Sorry, I still don't understand why this was introduced to begin with. Could 
you explain?


- Michael


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


On Nov. 25, 2015, 1: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, 1: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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40631]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 25, 2015, 1: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, 1: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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Benjamin Bannier]

> On Nov. 24, 2015, 9:42 a.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > 
> >
> > 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`.

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.


- Benjamin


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


On Nov. 25, 2015, 1: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, 1: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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Benjamin Bannier]

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

Ship it!


Ship It!

- Benjamin Bannier


On Nov. 25, 2015, 1: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, 1: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
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40631]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 24, 2015, 1:47 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Nov. 24, 2015, 1:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier 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 
> 78e7d145328c9f7aa9646fa7d6d92f834010053f 
>   src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Guangya Liu]

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



src/tests/mesos.hpp (line 87)


mistake? ;-)


- Guangya Liu


On 十一月 24, 2015, 9:19 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated 十一月 24, 2015, 9:19 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> 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 
> 78e7d145328c9f7aa9646fa7d6d92f834010053f 
>   src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

> On Nov. 24, 2015, 5:42 p.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > 
> >
> > 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`.

@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`.


- Klaus


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


On Nov. 24, 2015, 5:19 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Nov. 24, 2015, 5:19 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> 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 
> 78e7d145328c9f7aa9646fa7d6d92f834010053f 
>   src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Benjamin Bannier]

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



src/tests/mesos.hpp (line 87)


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`.


- Benjamin Bannier


On Nov. 24, 2015, 9:19 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Nov. 24, 2015, 9:19 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> 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 
> 78e7d145328c9f7aa9646fa7d6d92f834010053f 
>   src/tests/mesos.hpp a2a76f55f9a4091cccfccd9672c2d2897b34ddcd 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>