[Discuss-gnuradio] Using the private implementation ("pimpl") pattern for GNU Radio API classes

[Johnathan Corgan]

On Fri, Mar 9, 2012 at 12:31, Tom Rondeau <address@hidden> wrote:

> Now, Josh uses a structure where there is a public API class and an
> implementation class (impl). There are some really good reasons to do this,
> such as it would help us in moving towards an application binary interface
> (ABI). However, it is significantly different than what we do now. Johnathan
> Corgan and I have talked about this and are in agreement that this is a good
> direction to take in the future, but we want to introduce the changes in a
> reasonable and more systematic manner.

The private implementation ("pimpl") C++ coding pattern hides all
implementation details of classes from the users of those classes.  By
removing all private data members and methods from public GNU Radio
block header files, we can achieve a few benefits:

1) Fewer changes in GNU Radio would trigger recompilation of users'
applications.  Essentially, only actual API changes affect users, and
we only change those between "second digit" releases (3.1, 3.2, 3.3,
3.4, 3.5, etc.)

2) Include statements in block header files that only exist due to
usage in private block members go away.  Thus, user applications
compile faster and in some cases might even reduce user compile time
header file installation requirements.

3) SWIG interface files (.i) would simplify to just #including and
%including the public header file to generate the Python glue for
blocks.  The would also generate simpler SWIG and Python glue code.

4) It would move us closer to implementing an actual ABI for GNU
Radio, which would let GNU Radio users upgrade their binary
installations without recompiling or relinking their own applications.

5) Documentation generation systems like Doxygen wouldn't contain as
much implementation specific cruft, as they can point only at the
public header files to document the API, making it more clear to users
what their code should pay attention to and what irrelevant parts
might arbitrarily change.

Most of these benefit the users of GNU Radio, but they come at a
price--more work for the GNU Radio developers.  (One could argue that
more freedom to fiddle with GNU Radio internals without affecting
users benefits developers, though.)

The pimpl pattern comes in several variations.  Historically, GNU
Radio has used a related form of this, such as hiding the details of
inter-block communication by having a gr_block_detail class that
gr_block holds an instance of.  But the real benefits come from using
a particular pimpl coding pattern that creates a pure virtual block
class with API members, then having a (private, internal) derived
class with all the implementation details.

Josh Blum's implementation of gr-uhd created the first instance this
pattern showed up in GNU Radio, followed by his consolidation of the
various audio source/sink components into gr-audio.  Tom Rondeau
copied this pattern in creating gr-shd, and Tom Tsou and I used it for
parts of gr-vocoder.

So it has been creeping into GNU Radio already, and Tom and I think
the project would benefit from formally implementing this project
wide.

However, some implementation details (sorry) we think need to change.
Currently, the blocks following this pattern have a public header file
based on the block class name, like gr_foo_ff.h, which contains the
pure virtual class and nothing else.  Secondly, there exists the
implemention class, gr_foo_ff.cc, which contains both the
gr_foo_ff_impl class declaration and gr_foo_ff_impl member
implementations.

Having a class header file inside a .cc file, and then having the name
of the .cc file be different from the classes that are inside it,
makes it less readable.  Tom and I are proposing, if we do go to a
project-wide pimpl pattern, to have:

include/gr_foo_ff.h
lib/gr_foo_impl_ff.h
lib/gr_foo_impl_ff.cc

...as the implementation pattern for blocks inside GNU Radio.  (There
is a related, but orthogonal discussion, about directory layout which
is not being addressed here, nor am I addressing the also orthogonal
idea of using templates to eliminate the type suffixes, nor the idea
of using C++ namespaces to eliminate the file name prefixes.  One
discussion at a time.)

Also, making this change in no way would require GNU Radio users to
adopt this coding pattern; in fact user code wouldn't need to change
at all, and Python/GRC users would also remain unaffected.

If we decided to go ahead with this, we'd want to do so in a way which
creates the least disruption.  We'd want to plan ahead of time when
these things are changing, and do it in easily recognizable groups to
not confuse users reading our code base with a haphazard mix of the
two patterns.  Finally, as we make the change, we'd want to preserve
the other aspects of our coding style so the changes related to
converting to the pimpl pattern stand alone in the commits.

As the GNU Radio code base, its contributors, and user community grow
larger, Tom and I are trying to focus on more consistency and
readability of code, and are looking at established practices that
have benefitted other open source projects.  This involves both
"retrofitting" existing code and asking GNU Radio developers and
contributors to make more of an effort to "harmonize" their code with
existing and proposed best practices.

The above discussion of migrating to a project-wide pimpl coding
pattern is a part of this, and we'd like to get feedback from GNU
Radio users and developers as we evaluate it.

Johnathan