> On July 30, 2015, 4:45 p.m., Vinod Kone wrote: > > src/tests/containerizer/launcher.hpp, lines 19-37 > > <https://reviews.apache.org/r/36929/diff/1/?file=1024924#file1024924line19> > > > > why did you remove these headers? > > > > i think we decided to explicitly include all the headers that are used > > in a file instead of depending on transitive includes. > > Jie Yu wrote: > Is there a discussion somewhere? I think explicitly including all headers > make it hard to maintain (you'll need to adjust the header when the dependent > header changes). Also, it slows down the compilation. > > Vinod Kone wrote: > This is the pattern we followed in the code base. I think it improves > readability and avoids gotchas. For example, it is not clear at all that the > headers you kept provide a string or a vector definition (What if those > headers don't "#include <string>" at a later time?). This is the relevant > blurb from the google style guide > > ``` > You should include all the headers that define the symbols you rely upon > (except in cases of forward declaration). If you rely on symbols from bar.h, > don't count on the fact that you included foo.h which (currently) includes > bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its > intent to provide you the symbols of bar.h. However, any includes present in > the related header do not need to be included again in the related cc (i.e., > foo.cc can rely on foo.h's includes). > > ``` > > Some relevant discussion: > http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html > > Jie Yu wrote: > I have to disagree. C++ is not like Java, we don't have an automated tool > to help you include all necessary headers (java does). As a result, that > makes maintaining the includes very tedious. Any time someone is doing a > refactor, he/she has to manually check all the includes to make sure it's > up-to-date. Unless we have an automated tool to help you include all > necessary headers, I think having less includes makes it easy to maintain. > > The pattern I suggest here is: > ``` > base.hpp > > #include <string> > > class Base { > virtual void foo(const string& str); > }; > > derived.hpp > > #include "base.hpp" > > class Derived : public Base { > virtual void foo(const string& str); > }; > ``` > > We suggest don't include `<string>` in derived.hpp. This does not violate > google style as it states: "unless foo.h explicitly demonstrates its intent > to provide you the symbols of bar.h". I think here, base.hpp clearly > demonstrates its intent to provide you the symbols for all its public > interfaces.
It's fine if we want do it that way, but lets do that after we have a discussion on the dev list. It would be great to see if we can get some build improvement numbers. For now though I prefer we keep the codebase consistent. OK with you? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36929/#review93599 ----------------------------------------------------------- On July 30, 2015, 12:14 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36929/ > ----------------------------------------------------------- > > (Updated July 30, 2015, 12:14 a.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Fixed a few issues in test launcher header. > > > Diffs > ----- > > src/tests/containerizer/launcher.hpp > b80e84196f8b494e6e5a3c96c86f63fe432a7bb0 > > Diff: https://reviews.apache.org/r/36929/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
