mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 36929: Fixed a few issues in test launcher header.
Date Thu, 30 Jul 2015 21:50:30 GMT


> On July 30, 2015, 9:45 a.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.
> 
> Vinod Kone wrote:
>     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?

FWIW here is what include-what-you-use interprets what consititutes an "intent": https://code.google.com/p/include-what-you-use/wiki/WhatIsAUse,
which is in line with Jie's interpretation: if the dependency uses something on the "interface"
it's considered exporting the symbols whereas using it in its implementation is not.

+1 for seeking agreement from dev@.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36929/#review93599
-----------------------------------------------------------


On July 29, 2015, 5:14 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36929/
> -----------------------------------------------------------
> 
> (Updated July 29, 2015, 5:14 p.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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message