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