IWYU and move-only types passed by value?!

[Gabriel Charette]

Before move semantics, the IWYU (Include What You Use) rules were simple:

 1) if you declare a method that takes Foo by value you must include foo.h (so that users who merely forward a provided |foo| to the param don't have to include foo.h)

 2)  if you declare a method that returns Foo by value you must include foo.h (so that users who don't use the return value don't have to include foo.h)

 3) if you take Foo by pointer/ref only you can fwd-decl class Foo;

 4) if you use Foo you obviously need foo.h

With move-semantics however I just ran into an interesting scenario:

  void Bar(base::OnceClosure closure);

should a header providing this method include callback_forward.h or callback.h or neither?

Per rule #1 above it should include callback.h... but!...because of move-only semantics, any caller must either:

 a) std::move(closure) into the param.

 b) Have |closure| already be an r-value.

a) Forces the caller to include callback.h by IWYU rule #4 (for std::move)

b) Forces the API that provides the r-value to include callback.h by IWYU rule #2 and therefore the caller transitively already has foo.h

So including callback.h to define Bar() is redundant..? 

PS: Does that make callback_forward.h essentially be deprecated in the OnceClosure world?

[Daniel Cheng]

On Fri, Jul 21, 2017 at 3:15 PM Gabriel Charette <g...@chromium.org> wrote:

Before move semantics, the IWYU (Include What You Use) rules were simple:

 1) if you declare a method that takes Foo by value you must include foo.h (so that users who merely forward a provided |foo| to the param don't have to include foo.h)

 2)  if you declare a method that returns Foo by value you must include foo.h (so that users who don't use the return value don't have to include foo.h)

 3) if you take Foo by pointer/ref only you can fwd-decl class Foo;

 4) if you use Foo you obviously need foo.h

With move-semantics however I just ran into an interesting scenario:

  void Bar(base::OnceClosure closure);

should a header providing this method include callback_forward.h or callback.h or neither?

The idea behind callback_forward.h is to provide a lightweight header to #include in headers to avoid pulling in heavyweight headers via transitive dependencies. So the header should still be #include'ing callback_forward.h.

That being said, it seems like this ship sailed a long time ago: when we first migrated everything over to base::Bind/base::Callback, we tried to avoid including base/bind.h in header files as well... but it seems like this is pretty common now: https://cs.chromium.org/search/?q=%22base/bind.h%22+file:.h$&sq=package:chromium&type=cs

Daniel

Per rule #1 above it should include callback.h... but!...because of move-only semantics, any caller must either:

 a) std::move(closure) into the param.

 b) Have |closure| already be an r-value.

a) Forces the caller to include callback.h by IWYU rule #4 (for std::move)

b) Forces the API that provides the r-value to include callback.h by IWYU rule #2 and therefore the caller transitively already has foo.h

So including callback.h to define Bar() is redundant..? 

PS: Does that make callback_forward.h essentially be deprecated in the OnceClosure world?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LLT8TxFoxTMmGRgVrGXXNmo6HTjvKfAFCSgG5watahLPw%40mail.gmail.com.

[Gabriel Charette]

On Fri, Jul 21, 2017 at 8:03 PM Daniel Cheng <dch...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 3:15 PM Gabriel Charette <g...@chromium.org> wrote:

Before move semantics, the IWYU (Include What You Use) rules were simple:

 1) if you declare a method that takes Foo by value you must include foo.h (so that users who merely forward a provided |foo| to the param don't have to include foo.h)

 2)  if you declare a method that returns Foo by value you must include foo.h (so that users who don't use the return value don't have to include foo.h)

 3) if you take Foo by pointer/ref only you can fwd-decl class Foo;

 4) if you use Foo you obviously need foo.h

With move-semantics however I just ran into an interesting scenario:

  void Bar(base::OnceClosure closure);

should a header providing this method include callback_forward.h or callback.h or neither?

The idea behind callback_forward.h is to provide a lightweight header to #include in headers to avoid pulling in heavyweight headers via transitive dependencies. So the header should still be #include'ing callback_forward.h.

Read on below though, my point is callback_forward.h doesn't do anything here because the users of this API are already guaranteed to have callback.h included. So either we include callback.h (redundant but closer to IWYU rules) or we include neither. callback_forward.h is incorrect.

[Yuri Wiitala]

Related question: Do we have any IWYU tools, say, for scanning existing code? Or, even, auto-fixing header files? I wouldn't mind running such a tool every few months to "keep my house in order."

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LLBzqO%2BdZyf8sMSuH7m-Qnn6LMCE3XTk-7_gu9_VYo%3D9g%40mail.gmail.com.

[Daniel Cheng]

On Fri, Jul 21, 2017 at 6:24 PM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 8:03 PM Daniel Cheng <dch...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 3:15 PM Gabriel Charette <g...@chromium.org> wrote:

Before move semantics, the IWYU (Include What You Use) rules were simple:

 1) if you declare a method that takes Foo by value you must include foo.h (so that users who merely forward a provided |foo| to the param don't have to include foo.h)

 2)  if you declare a method that returns Foo by value you must include foo.h (so that users who don't use the return value don't have to include foo.h)

 3) if you take Foo by pointer/ref only you can fwd-decl class Foo;

 4) if you use Foo you obviously need foo.h

With move-semantics however I just ran into an interesting scenario:

  void Bar(base::OnceClosure closure);

should a header providing this method include callback_forward.h or callback.h or neither?

The idea behind callback_forward.h is to provide a lightweight header to #include in headers to avoid pulling in heavyweight headers via transitive dependencies. So the header should still be #include'ing callback_forward.h.

Read on below though, my point is callback_forward.h doesn't do anything here because the users of this API are already guaranteed to have callback.h included. So either we include callback.h (redundant but closer to IWYU rules) or we include neither. callback_forward.h is incorrect.

There can be consumers of the header that don't use Bar(): in that case, callback_forward.h is sufficient.

Daniel

[Daniel Cheng]

On Fri, Jul 21, 2017 at 6:28 PM Yuri Wiitala <m...@chromium.org> wrote:

Related question: Do we have any IWYU tools, say, for scanning existing code? Or, even, auto-fixing header files? I wouldn't mind running such a tool every few months to "keep my house in order."

We've previously attempted to run IWYU. IWYU did some weird things, trying to remove some headers it shouldn't, and adding some surprising headers (like stdbool.h).

https://bugs.chromium.org/p/chromium/issues/detail?id=259043 has some history.

If someone's interested in doing this, I would say it'd probably be easier to write a custom tool that just tries to make sure a subset of headers are included as appropriate:

- C and C++ standard library

- //base

- //mojo

- other things depended on by significant parts of the codebase (not sure what those would be, but suggestions welcome)

If we covered enough core headers, I think we'd get a large portion of the benefit without the weirdness of IWYU.

Daniel

[Gabriel Charette]

Le ven. 21 juil. 2017 22:14, Daniel Cheng <dch...@chromium.org> a écrit :

On Fri, Jul 21, 2017 at 6:24 PM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 8:03 PM Daniel Cheng <dch...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 3:15 PM Gabriel Charette <g...@chromium.org> wrote:

Before move semantics, the IWYU (Include What You Use) rules were simple:

 1) if you declare a method that takes Foo by value you must include foo.h (so that users who merely forward a provided |foo| to the param don't have to include foo.h)

 2)  if you declare a method that returns Foo by value you must include foo.h (so that users who don't use the return value don't have to include foo.h)

 3) if you take Foo by pointer/ref only you can fwd-decl class Foo;

 4) if you use Foo you obviously need foo.h

With move-semantics however I just ran into an interesting scenario:

  void Bar(base::OnceClosure closure);

should a header providing this method include callback_forward.h or callback.h or neither?

The idea behind callback_forward.h is to provide a lightweight header to #include in headers to avoid pulling in heavyweight headers via transitive dependencies. So the header should still be #include'ing callback_forward.h.

Read on below though, my point is callback_forward.h doesn't do anything here because the users of this API are already guaranteed to have callback.h included. So either we include callback.h (redundant but closer to IWYU rules) or we include neither. callback_forward.h is incorrect.

There can be consumers of the header that don't use Bar(): in that case, callback_forward.h is sufficient.

But isn't this a valid argument for every parameter being passed by value and goes against IWYU rule #1 above? Or maybe it's fine for move-only types passed by value because if the caller does use Bar() he's responsible for callback.h as highlighted below?

[Peter Kasting]

On Sat, Jul 22, 2017 at 6:00 AM, Gabriel Charette <g...@chromium.org> wrote:

Le ven. 21 juil. 2017 22:14, Daniel Cheng <dch...@chromium.org> a écrit :

On Fri, Jul 21, 2017 at 6:24 PM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 8:03 PM Daniel Cheng <dch...@chromium.org> wrote:

On Fri, Jul 21, 2017 at 3:15 PM Gabriel Charette <g...@chromium.org> wrote:

Before move semantics, the IWYU (Include What You Use) rules were simple:

 1) if you declare a method that takes Foo by value you must include foo.h (so that users who merely forward a provided |foo| to the param don't have to include foo.h)

 2)  if you declare a method that returns Foo by value you must include foo.h (so that users who don't use the return value don't have to include foo.h)

 3) if you take Foo by pointer/ref only you can fwd-decl class Foo;

 4) if you use Foo you obviously need foo.h

With move-semantics however I just ran into an interesting scenario:

  void Bar(base::OnceClosure closure);

should a header providing this method include callback_forward.h or callback.h or neither?

The idea behind callback_forward.h is to provide a lightweight header to #include in headers to avoid pulling in heavyweight headers via transitive dependencies. So the header should still be #include'ing callback_forward.h.

Read on below though, my point is callback_forward.h doesn't do anything here because the users of this API are already guaranteed to have callback.h included. So either we include callback.h (redundant but closer to IWYU rules) or we include neither. callback_forward.h is incorrect.

There can be consumers of the header that don't use Bar(): in that case, callback_forward.h is sufficient.

But isn't this a valid argument for every parameter being passed by value and goes against IWYU rule #1 above? Or maybe it's fine for move-only types passed by value because if the caller does use Bar() he's responsible for callback.h as highlighted below?

For reference,  where do these rules come from?  They're not listed on include-what-you-use.org and I only think of IWYU as #4.  I tend to think of #1 and #2 as actively harmful.

PK

[Michael Giuffrida]

This. #1 just doesn't seem accurate: the convention we have AFAIK is to forward-declare parameters taken by value, which is what callback_foward.h does.

callback.h also says:

// NOTE: Header files that do not require the full definition of Callback or
// Closure should #include "base/callback_forward.h" instead of this file.

so in your example, since the function declaration doesn't *require* the full definition of base::OnceClosure, you shouldn't include the full header.

 

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFCdKmcxoGeRrFq9kFgg7d3mYP8wKV-8HAXm7QmRt297QA%40mail.gmail.com.

[Gabriel Charette]

Somehow I thought we had them but I can't find them anywhere so I guess they're my own homegrown IWYU rules...

IMO these rules make for better APIs (require less redundant includes from users).

e.g. consider base/task_scheduler/post_task.h. It includes task_traits.h because you'll need to pass it in. It also provides ref_counted.h and the various task_runner.h it returns so that you can use the API without having to re-include everything it hands off (even you're the one "using it").

+John Abd-El-Malek felt strongly about avoiding redundant includes as well IIRC.

 

PK

[Peter Kasting]

They make for fewer redundant #includes if people are actually using the things in your header, but it seems like many headers:

* Include a lot of stuff, so you're only maybe going to use a little of it

* Get #included transitively, because some other header you need needed this

In those scenarios, I think these rules lead to #includes being pulled in to too many places.  I'm a bigger fan of "include as little as possible until the leaf node, and pull everything in at the leaf".

PK