Using the private implementation ("pimpl") pattern for GNU Radio API classes

On Fri, Mar 9, 2012 at 12:31, Tom R. [email protected] 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 B.'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 R.
copied this pattern in creating gr-shd, and Tom T. 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

I’m a big fan of this (in some way or another, I’ve been doing similar
stuff already in gr-specest etc.). It also really eases up
interoperation with external libraries.

Also, I really don’t think this comes as extra effort. Code-generation
tools can help here a lot (the files containing the block definition
would be pretty similar in most cases, anyway). If anything, it makes
coding simpler because you’re dealing with less problems per file.

On Mon, Mar 12, 2012 at 12:31:53PM -0700, Johnathan C. wrote:

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

This might seem minor, but I like the first naming scheme you proposed
much better than the second (gr_foo_ff_impl > gr_foo_impl_ff).
I think of ‘foo_ff’ as the block name, which would be disrupted, and an
intuitive glob for impl-header-files is (in my head) “*impl.h”.

Or, to put it in other terms, imagine you’re searching for all the
foo_ff related files, then find . -name “foo_ff” will not work.

And that’s really all the criticism I could possibly find. This is a
great idea.

MB


Karlsruhe Institute of Technology (KIT)
Communications Engineering Lab (CEL)

Dipl.-Ing. Martin B.
Research Associate

Kaiserstraße 12
Building 05.01
76131 Karlsruhe

Phone: +49 721 608-43790
Fax: +49 721 608-46071
www.cel.kit.edu

KIT – University of the State of Baden-Württemberg and
National Laboratory of the Helmholtz Association

On Tue, Mar 13, 2012 at 01:10, Martin B. [email protected]
wrote:

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

This might seem minor, but I like the first naming scheme you proposed
much better than the second (gr_foo_ff_impl > gr_foo_impl_ff).
I think of ‘foo_ff’ as the block name, which would be disrupted, and an
intuitive glob for impl-header-files is (in my head) “*impl.h”.

After typing and proofing that long email, it’s funny that I could
leave in such a mistake.

It was intended that the filenames listed in the three lines be the
same as the class names two paragraphs above, that was the whole
point. So yes, exactly as you said.

Thanks for the feedback.

Johnathan

On Tue, Mar 13, 2012 at 09:12, Tom R. [email protected] wrote:

…and gr-fcd (which should be added soon).

The gr-fcd branch is just getting final testing and is ready for
merging. I can do that, to get it into master as soon as possible,
then add a commit to the branch which makes the change to the pimpl
pattern, for people to look at.

The post-merge commit can still eventually merge again into master
without affecting users, as the conversion does not alter the public
portions of the class header.

Johnathan

On Tue, Mar 13, 2012 at 11:12 AM, Johnathan C. <
[email protected]> wrote:

Having a class header file inside a .cc file, and then having the name
I think of ‘foo_ff’ as the block name, which would be disrupted, and an

Johnathan

Good catch, Martin, that pattern definitely makes the most sense.

Once we’re settled on a general pattern here, we can start planning the
changes. We have a few of the new top-level block structures done that
are
nicely separate and self-contained. We could plan on making the changes
there, first. This include gr-digital, gr-vocoder, gr-audio, gr-shd,
gr-uhd, and gr-fcd (which should be added soon). This should only take a
small amount of effort to do. We’ll be leaving everything in
gnuradio-core
alone for now, though. There’s other discussions to be had for what we
plan
on with that.

So we’ll ask any developers to try to keep up as we change things.
Nothing’s happening right now, and I’d like to try to make this switch
happen almost at once. When we move over some of the current directories
to
this new style, we’ll try to push it as one change to master/next. At
that
point, anyone working on these components would be asked to start using
that model. We’ll put it on the wiki as well. Of course, if you’re
actively
developing, don’t worry much about it; I’ll volunteer my time to do the
conversion of a new and working block for the time being. I don’t want
this
to be a huge burden.

Thanks for the feedback.

Tom

At the moment, I don’t have any suggestions for changes to Johnathan’s
proposed structure, but I just wanted to voice support for this
initiative.

I think it’s a great idea, and I think both users and developers will
benefit from it in the future.

Cheers,
Ben

On Tue, Mar 13, 2012 at 11:36 AM, Johnathan C. <

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs