Jie, I thought that duplicate includes of headers don't have a significant impact on compile times given our include guards, why do you say it slows down the compilation?
e.g. https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html On Thu, Jul 30, 2015 at 12:57 PM, Vinod Kone <vinodk...@gmail.com> wrote: > > > > 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 > > > > > >
