aha, I just thought it might slow down the compilation. But looks like it will not given the optimization. I guess clang should have the same optimization as well.
The burden of updating the headers while doing refactor is real. It'll be really cool if we can automate this. BTW: the code base is already not consistent. For example: https://github.com/apache/mesos/blob/master/src/slave/containerizer/isolators/filesystem/posix.hpp We don't include future.hpp, option.hpp, etc. - Jie On Thu, Jul 30, 2015 at 1:08 PM, Benjamin Mahler <[email protected]> wrote: > 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 <[email protected]> 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 >> > >> > >> >> >
