Forward declaring base::Callback and base::Closure

[Erik Wright]

TL:DR: Wondering if it's distasteful to forward declare typdefs and templates such as:

namespace base {

  template <typename Sig> class Callback;

  typedef Callback<void(void)> Closure;

}  // namespace base

Hi All,

Are there any strong opinions on whether class definitions that accept callbacks should forward declare Callback (and, if necessary, Closure) when possible, rather than #including callback.h?

If the preference is to not forward-declare, what is the defining characteristic that separates this from cases where we do forward declare? 

-Erik

[William Chan (陈智昌)]

On Tue, Oct 4, 2011 at 10:00 AM, Erik Wright <erikw...@chromium.org> wrote:

TL:DR: Wondering if it's distasteful to forward declare typdefs and templates such as:

namespace base {

  template <typename Sig> class Callback;

  typedef Callback<void(void)> Closure;

}  // namespace base

Since we're talking about style, let me be nitpicky and point out you shouldn't be indenting here.

 

Hi All,

Are there any strong opinions on whether class definitions that accept callbacks should forward declare Callback (and, if necessary, Closure) when possible, rather than #including callback.h?

Internally in Google C++ style discussions, there was a debate on forward declarations of templates, and forward declarations in general. Basically, forward declarations are brittle but they help in breaking compile-time dependencies. In general you should use them. And for the particular example of google3's callback headers, which are pretty similar to ours in general, forward declarations are useful because the header files are quite substantial.

In order to stay consistent with internal google practice which may eventually make its way into the Google C++ style guide, I favor forward declaring callbacks.

 

If the preference is to not forward-declare, what is the defining characteristic that separates this from cases where we do forward declare? 

-Erik

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Matt Perry]

Here's a silly idea: if we're going to standardize on forward declaring callbacks, what if we added a callback_forward.h that did the forward declares properly, and use that instead?

[William Chan (陈智昌)]

On Tue, Oct 4, 2011 at 11:34 AM, Matt Perry <mpcom...@chromium.org> wrote:

Here's a silly idea: if we're going to standardize on forward declaring callbacks, what if we added a callback_forward.h that did the forward declares properly, and use that instead?

I don't think it's silly at all, and is indeed roughly what csilvers recommended internally (he hoped for a clang automated forward declaration header generator, but I think that's beyond what we need). I'm for it.

[Darin Fisher]

On Tue, Oct 4, 2011 at 11:34 AM, Matt Perry <mpcom...@chromium.org> wrote:

Here's a silly idea: if we're going to standardize on forward declaring callbacks, what if we added a callback_forward.h that did the forward declares properly, and use that instead?

I had exactly the same thought.  This seems like a good idea to me.

[Erik Wright]

Great. I'll send a CL for this.

[Albert J. Wong (王重傑)]

To quantify with data, callback.h and callback_internal.h, stripping comments, totals 363 lines with 13 declarations (hand counted).

   cat base/callback.h base/callback_internal.h | grep -v ' *//' | wc -l     => 363

The include tree above it is pretty small.  Running through the the proprocessor, we get 5441 lines:

    g++ -I. -E base/callback.h | wc -l  => 5442

My inclination is that we really won't gain much either way, so my vote is *shrug*.

The more important thing is to keeping bind.h out of header files which should only be required if we're writing templates that somehow wrap a base::Bind() call.

-Albert

[William Chan (陈智昌)]

On Tue, Oct 4, 2011 at 11:50 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

To quantify with data, callback.h and callback_internal.h, stripping comments, totals 363 lines with 13 declarations (hand counted).

   cat base/callback.h base/callback_internal.h | grep -v ' *//' | wc -l     => 363

The include tree above it is pretty small.  Running through the the proprocessor, we get 5441 lines:

    g++ -I. -E base/callback.h | wc -l  => 5442

My inclination is that we really won't gain much either way, so my vote is *shrug*.

I admit I don't have a good intuition for how much these 5000 LoC matter. It certainly seems significantly larger than the 30 lines or so required for the 6 forward declarations. If you think it doesn't matter, I'd be willing to accept that.

[Erik Wright]

The other question is how often those 5442 lines are changing, since each one is now going to mean rebuilding the world.

[Albert J. Wong (王重傑)]

cat base/callback.h base/callback_internal.h | grep '#include'

#include "base/callback_internal.h"

#include "base/template_util.h"

#include <stddef.h>

#include "base/base_export.h"

#include "base/memory/ref_counted.h"

Those are all very stable.  Much of the 5442 lines are standard libraries.

Having said that, if people are worried, I think we should just add the forward decl.

That also insulates us from future possible pollution of callback.h and callback_internal.h and discussing further probably costs more than we'd gain. :)

-Albert

[Erik Wright]

The answer to my question is that 9 CLs have affected that include tree since June 1st (i.e. touched every two weeks):

git log --oneline --since=6/1/2011 `(files=base/callback.h; while [ -n "$files" ]; do for file in $files; do echo $file; done; files=\`sed -n -e 's/^#include "\(.*\)"$/\1/p' $files\`; done) | sort | uniq | tr '\n' ' '` | wc -l

But I don't know if these questions are really appropriate. After all, many includes are probably only a few thousand lines, and perhaps only changing once every two weeks. But if I have even a small number of these things that are included across the source tree, it's death by a thousand cuts...

-Erik

[Albert J. Wong (王重傑)]

sgetm. 

Forward declare away!