tags:

views:

586

answers:

6

I'm using shared_ptr and STL extensively in a project, and this is leading to over-long, error-prone types like shared_ptr< vector< shared_ptr<const Foo> > > (I'm an ObjC programmer by preference, where long names are the norm, and still this is way too much.) It would be much clearer, I believe, to consistently call this FooListPtr and documenting the naming convention that "Ptr" means shared_ptr and "List" means vector of shared_ptr.

This is easy to typedef, but it's causing headaches with the headers. I seem to have several options of where to define FooListPtr:

  • Foo.h. That entwines all the headers and creates serious build problems, so it's a non-starter.
  • FooFwd.h ("forward header"). This is what Effective C++ suggests, based on iosfwd.h. It's very consistent, but the overhead of maintaining twice the number of headers seems annoying at best.
  • Common.h (put all of them together into one file). This kills reusability by entwining a lot of unrelated types. You now can't just pick up one object and move it to another project. That's a non-starter.
  • Some kind of fancy #define magic that typedef's if it hasn't already been typedefed. I have an abiding dislike for the preprocessor because I think it makes it hard for new people to grok the code, but maybe....
  • Use a vector subclass rather than a typedef. This seems dangerous...

Are there best practices here? How do they turn out in real code, when reusability, readability and consistency are paramount?

I've marked this community wiki if others want to add additional options for discussion.

+3  A: 

I would go with a combined approach of forward headers and a kind of common.h header that is specific to your project and just includes all the forward declaration headers and any other stuff that is common and lightweight.

You complain about the overhead of maintaining twice the number of headers but I don’t think this should be too much of a problem: the forward headers usually only need to know a very limited number of types (one?), and sometimes not even the full type.

You could even try auto-generating the headers using a script (this is done e.g. in SeqAn) if there are really that many headers.

Konrad Rudolph
I work mostly on code that is used by several projects, so almost none of this specific to a single project. A single "common.h" makes it very hard for multiple projects to reuse components.
Rob Napier
@Konrad : does SeqAn use boost and std libraries ? What does the script you mention do ?
Benoît
@Benoît: SeqAn uses STL but (unfortunately, IMHO) no Boost libraries. As for the script, it just parses all the header files (but then, SeqAn is headers only) and generates one large file containing forward declarations of all exported types and functions.
Konrad Rudolph
@Konrad : i agree it's too bad that they don't use boost libraries (especially for graph algorithms!). Is the script standard in some way ?
Benoît
I had never personally looked at the script before now; honestly, it looks like a mess. But on the other hand, it works very reliable and copes with complex template definitions. And it looks as if it could be reused or customized without too much trouble. If you want to take a look: http://svn.mi.fu-berlin.de/seqan/trunk/seqan/misc/build_forwards.py
Konrad Rudolph
+1  A: 

Unfortunately with typedefs you have to choose between not ideal options for your header files. There are special cases where option one (right in the class header) works well, but it sounds like it won't work for you. There are also cases where the last option works well, but it's usually where you are using the subclass to replace a pattern involving a class with a single member of type std::vector. For your situation, I'd use the forward declaring header solution. There's extra typing and overhead, but it wouldn't be C++ otherwise, right? It keeps things separate, clean and fast.

Michael Daum
It does seem to be the most consistent and scalable approach. "it wouldn't be C++ otherwise...." It does seem to be the way. I'm an ObjC programmer usually. People complain about the long names in ObjC, but I find them very comfortable and don't mind them at all. But in C++ I feel I spend half my time doing verbose busy-work, or else make the code inconsistent and hard to maintain in order to avoid the extra typing.
Rob Napier
+1  A: 

+1 for documenting the typedef conventions.

  • Foo.h - can you detail the problems you have with that?
  • FooFwd.h - I'd not use them generally, only on "obvious hotspots". (Yes, "hotspots" are hard to determine). It doesn't change the rules IMO because when you do introduce a fwd header, the associated typedefs from foo.h move there.
  • Common.h - cool for small projects, but doesn't scale, I do agree.
  • Some kind of fancy #define... PLEASE NO!...
  • Use a vector subclass - doesn't make it better. You might use containment, though.

So here the prelimenary suggestions (revised from that other question..)

  1. Standard type headers <boost/shared_ptr.hpp>, <vector> etc. can go into a precompiled header / shared include file for the project. This is not bad. (I personally still include them where needed, but that works in addition to putting them into the PCH.)

  2. If the container is an implementation detail, the typedefs go where the container is declared (e.g. private class members if the container is a private class member)

  3. Associated types (like FooListPtr) go to where Foo is declarated, if the associated type is the primary use of the type. That's almost always true for some types - e.g. shared_ptr.

  4. If Foo gets a separate forward declaration header, and the associated type is ok with that, it moves to the FooFwd.h, too.

  5. If the type is only associated with a particular interface (e.g. parameter for a public method), it goes there.

  6. If the type is shared (and does not meet any of the previous criteria), it gets its own header. Note that this also means to pull in all dependencies.

It feels "obvious" for me, but I agree it's not good as a coding standard.

peterchen
good points in general, but re #1: i don't recommend including the lot of standard libraries in a common header. the worst offense is the amount of private static data and definitions produced.
Justin
@Justin - Definitions - agreed, but that's what precompiled headers are for. but private static data? there's close to none in the standard headers (as it would be per translation unit anyway). Or do you mean the problems with early template support implementations? Unless that's a known and proven problem for your compiler, there's no point in sticking to it.
peterchen
i don't use precompiled headers. rationale: not enough commonality, and i often use combined builds so there is in many cases one file compiled per package. also, precompiled headers don't scale particularly well in distributed build systems. as far as static data, one obvious declaration that shows up is input/output streams (included by `iostream`). i lost about 20% of some binaries *after* removing superfluous includes from standard libraries (measured using a relatively modern implementation of std libs shipped with apple's gcc). the problems obviously extend beyond binary size.
Justin
A: 

We do the same thing here. This is what has worked for us:

  1. If it's something simple as typdef unordered_map<Foo, shared_ptr<Bar> > FooBarMap in which the dependencies on Foo and Bar go hand in hand within a project we'll happily place the typedefs in a single file with other helpers specific to those functions. Instead of coming up with a name like FooBarHelper, we use "FooBarTypes."
  2. If it's something even more basic like typedef long KeyId and unordered_map<KeyID,Foo> we'll actually leave it the same file as the definition of Foo. It doesn't matter if another class will not be using unordered_map, it's available for future refactoring without having to track something down.

The biggest pitfalls need to watch out for are happy-go-lucky programmers that don't think about coupling between classes. We make strict requirements that if a typedef requires 2 or more classes from within our own projects (excluding STL and other built-in library functions) then that typedef can only be included if the class which will include it requires those classes.

  1. It's actually quite simple to put in a script to check this within your build process. I very much suggest it.
  2. Have the script report to your automatic build processes reporting mechanism as a failure.
wheaties
+2  A: 

I'm programming on a project which sounds like it uses the common.h method. It works very well for that project.

There is a file called ForwardsDecl.h which is in the pre-compiled header and simply forward-declares all the important classes and necessary typedefs. In this case unique_ptr is used instead of shared_ptr, but the usage should be similar. It looks like this:

// Forward declarations
class ObjectA;
class ObjectB;
class ObjectC;

// List typedefs
typedef std::vector<std::unique_ptr<ObjectA>> ObjectAList;
typedef std::vector<std::unique_ptr<ObjectB>> ObjectBList;
typedef std::vector<std::unique_ptr<ObjectC>> ObjectCList;

This code is accepted by Visual C++ 2010 even though the classes are only forward-declared (the full class definitions are not necessary so there's no need to include each class' header file). I don't know if that's standard and other compilers will require the full class definition, but it's useful that it doesn't: another class (ObjectD) can have an ObjectAList as a member, without needing to include ObjectA.h - this can really help reduce header file dependencies!

Maintenance is not particularly an issue, because the forwards declarations only need to be written once, and any subsequent changes only need to happen in the full declaration in the class' header file (and this will trigger fewer source files to be recompiled due to reduced dependencies).

Finally it appears this can be shared between projects (I haven't tried myself) because even if a project does not actually declare an ObjectA, it doesn't matter because it was only forwards declared and if you don't use it the compiler doesn't care. Therefore the file can contain the names of classes across all projects it's used in, and it doesn't matter if some are missing for a particular project. All that is required is the necessary full declaration header (e.g. ObjectA.h) is included in any source (.cpp) files that actually use them.

AshleysBrain
+1  A: 

I'm using shared_ptr and STL extensively in a project, and this is leading to over-long, error-prone types like shared_ptr< vector< shared_ptr > > (I'm an ObjC programmer by preference, where long names are the norm, and still this is way too much.) It would be much clearer, I believe, to consistently call this FooListPtr and documenting the naming convention that "Ptr" means shared_ptr and "List" means vector of shared_ptr.

for starters, i recommend using good design structures for scoping (e.g., namespaces) as well as descriptive, non-abbreviated names for typedefs. FooListPtr is terribly short, imo. nobody wants to guess what an abbreviation means (or be surprised to find Foo is const, shared, etc.), and nobody wants to alter their code simply because of scope collisions.

it may also help to choose a prefix for typedefs in your libraries (as well as other common categories).

it's also a bad idea to drag types out of their declared scope:

namespace MON {
namespace Diddy {
class Foo;
} /* << Diddy */

/*...*/
typedef Diddy::Foo Diddy_Foo;

} /* << MON */

there are exceptions to this:

  • an entirely ecapsualted private type
  • a contained type within a new scope

while we're at it, using in namespace scopes and namespace aliases should be avoided - qualify the scope if you want to minimize future maintentance.

This is easy to typedef, but it's causing headaches with the headers. I seem to have several options of where to define FooListPtr:

Foo.h. That entwines all the headers and creates serious build problems, so it's a non-starter.

it may be an option for declarations which really depend on other declarations. implying that you need to divide packages, or there is a common, localized interface for subsystems.

FooFwd.h ("forward header"). This is what Effective C++ suggests, based on iosfwd.h. It's very consistent, but the overhead of maintaining twice the number of headers seems annoying at best.

don't worry about the maintenance of this, really. it is a good practice. the compiler uses forward declarations and typedefs with very little effort. it's not annoying because it helps reduce your dependencies, and helps ensure that they are all correct and visible. there really isn't more to maintain since the other files refer to the 'package types' header.

Common.h (put all of them together into one file). This kills reusability by entwining a lot of unrelated types. You now can't just pick up one object and move it to another project. That's a non-starter.

package based dependencies and inclusions are excellent (ideal, really) - do not rule this out. you'll obviously have to create package interfaces (or libraries) which are designed and structured well, and represent related classes of components. you're making an unnecessary issue out of object/component reuse. minimize the static data of a library, and let the link and strip phases do their jobs. again, keep your packages small and reusable and this will not be an issue (assuming your libraries/packages are well designed).

Some kind of fancy #define magic that typedef's if it hasn't already been typedefed. I have an abiding dislike for the preprocessor because I think it makes it hard for new people to grok the code, but maybe....

actually, you may declare a typedef in the same scope multiple times (e.g., in two separate headers) - that is not an error.

declaring a typedef in the same scope with different underlying types is an error. obviously. you must avoid this, and fortunately the compiler enforces that.

to avoid this, create a 'translation build' which includes the world - the compiler will flag declarations of typedeffed types which don't match.

trying to sneak by with minimal typedefs and/or forwards (which are close enough to free at compilation) is not worth the effort. sometimes you'll need a bunch of conditional support for forward declarations - once that is defined, it is easy (stl libraries are a good example of this -- in the event you are also forward declaring template<typename,typename>class vector;).

it's best to just have all these declarations visible to catch any errors immediately, and you can avoid the preprocessor in this case as a bonus.

Use a vector subclass rather than a typedef. This seems dangerous...

a subclass of std::vector is often flagged as a "beginner's mistake". this container was not meant to be subclassed. don't resort to bad practices simply to reduce your compile times/dependencies. if the dependency really is that significant, you should probably be using PIMPL, anyways:

// <package>.types.hpp
namespace MON {
class FooListPtr;
}

// FooListPtr.hpp
namespace MON {
class FooListPtr {
    /* ... */
private:
    shared_ptr< vector< shared_ptr<const Foo> > > d_data;
};
}

Are there best practices here? How do they turn out in real code, when reusability, readability and consistency are paramount?

ultimately, i've found a small concise package based approach the best for reuse, for reducing compile times, and minimizing dependence.

Justin