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 <benjamin.mah...@gmail.com>
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 <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
>> >
>> >
>>
>>
>

Reply via email to