Proposal: Adding C++ utilities to ign-cmake

[osrf-migration]

Original report (archived issue) by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

---

Apologies in advance for the wall-of-text.

The original motivation for creating ign-cmake was to systematically ensure consistent build behavior across all the ignition projects, in a way that is maintainable and scalable.

Even though the ign-cmake package is still in development, it's had some very tangible benefits: we now have four projects (ign-math, ign-common, ign-msgs, and ign-transport) with robust build systems across all of our supported platforms and consistent package behavior across all of the projects. If we ever decide to change our packaging behavior or if we ever need to fix a build system bug, we can take care of it in ign-cmake, and all the ignition projects will benefit the next time they configure. Future and upcoming projects will experience these benefits as well.

However, we want to have consistency across all the ignition projects in more ways than just their build system and packaging. The most obvious example is the PIMPL pattern, which is a design pattern that we aim to use across all the ignition projects to achieve maximum flexibility in our development cycles without needing to be concerned about breaking ABI.

The forcing function that prompted me to make this proposal was in a recent ign-transport pull request, we found ourselves repeatedly writing boilerplate move and copy constructors, plus move and copy assignment operators for just about every darn interface class (here's a sampling from that one pull request: 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19).

The worst part is, even with all that effort in manually creating all those constructors and assignment operators, some of the classes in that pull request still don't have proper copy and move semantics, simply because we have not yet noticed that their copy/move constructors/operators are missing.

That's when I remembered some design utilities promoted by Scott Meyers which makes the PIMPL pattern so very much more bearable. Here's a tl;dr of that article:

Instead of using a plain std::unique_ptr<Implementation> to contain the implementation object, you can have a impl_ptr<Implementation> which takes care of creating the copy and move constructors for your interface class. To make that more concrete, your class definition would look like this:

class MyClass
{
  public: /* Interface functions go here */

  private: class Implementation;
  private: ImplPtr<Implementation> dataPtr;
};

Then in the source file, your constructor would look like this:

MyClass::MyClass( /* constructor args */ )
  : dataPtr(MakeImpl<Implementation>(/* constructor args */))
{
  // Do nothing
}

This will allow MyClass to automatically create a copy constructor, move constructor, copy operator, and move operator, and all you have to do is replace std::unique_ptr with ImplPtr and new Implementation with MakeImpl<Implementation>. Note that we can also have a UniqueImplPtr which will allow moving but not copying, for classes that are not copyable. The catch, of course, is you need to have access to the header that defines ImplPtr and MakeImpl.

Since it's a single header file, the brute force approach would be to write the header and then drop it into each project. That's a viable solution if we're confident that we'll never ever have to make any changes to the implementation, but that's a very risky assumption to make.

Ideally, we would want the ImplPtr to be available in every project from a single source. We already have a project which is a required dependency of every ignition project: ign-cmake. If we were to consider the goal of ign-cmake to be achieving consistent, maintainable, scalable development (not just build+packaging behavior) across all the ignition projects, then it would make sense to have it provide a small set of C++ utilities that could be used across all projects to keep behavior and development consistent between them.

I can think of three utilities which would make sense to share across all projects right away: (1) the pimpl class mentioned here, (2) the warning suppression system which is currently in ign-common, and (3) the Console utility currently in ign-common. These are things that all project would benefit from:

1. Anything that can make PIMPL easier to stomach is a good thing.

2. We're repeatedly dealing with the same compiler warnings across every project. This would give us a clean way to handle those consistently everywhere.

3. Having the Console class and its error/warning macros as base utilities would allow us to have consistent status, warning, and error messaging behavior across all the libraries, instead of having some print proper messages while others print plain, unformatted messages.

We could also consider the Filesystem from ign-common, but I don't think that's as basic of a utility (and it will probably be replaced by the C++17 filesystem soon anyway).

Why not just use ign-common?

Right now, ign-common would be more aptly named ign-miscellaneous. Most of its content is not common to all projects, and it would be unreasonable to have all projects depend on it, even after it's been split into components.

ign-cmake is already a required dependency. Putting these utilities into ign-cmake will make them accessible to all projects right away, without introducing any new dependencies.

Then could we really call this project ign-cmake anymore?

Maybe not.

I would propose that we add these utilities to ign-cmake as a library called ignition-utilities (or maybe ignition-utils, or some variation on that). When each project calls find_package(ignition-cmake1) it will import a target for the ignition-utilities library, and ign-cmake can automatically link each project library against the utilities target. Consumer projects wouldn't have to change anything in their build scripts.

Then, when we're approaching the hypothetical release of ignition-cmake1, we can decide whether we want to keep using the name ignition-cmake. Maybe by then, ign-common will be rid of its miscellaneous components, and we can have ign-cmake take over the name ign-common. Otherwise, we could consider renaming ign-cmake to something like ign-utilities, ign-utils, ign-root, ign-base, or any other suggestions that people have. Or we could just keep the name as ign-cmake and accept the slight misnomer of the project.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

This is a summary of my off the cuff opinions about parts of your proposal.

Proposal	Cost	Benefit
Put warning suppression macros in header installed from ign-cmake	install an extra file, unlikely to affect API/ABI, easy	code easier to read
Put Implementation class in header file installed from ign-cmake	still just installing a header file, but any changes could affect API/ABI	easier to implement move semantics
Put Console class in ign-cmake	requires compiling and distributing a library with ign-cmake	easier to print consistent console warnings
Rename ign-cmake	lots of work	subjective

I could easily be convinced of the first two rows since they are header-only; I'm skeptical of putting compiled code in ign-cmake, and I'm even more skeptical about renaming ign-cmake from a cost/benefit perspective.

[osrf-migration]

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

---

Thanks for the chart, I think that's a very helpful overview. Just a few comments on it:

Warning suppression will definitely never affect API or ABI since it only introduces compiler directives that relate to whether or not specific warnings get emitted by the compiler. They have zero effect on how the code itself gets interpreted. Any warning suppression methods that go beyond that would be outside the scope of the suppression macros.

Besides "easiness", I'd like to emphasize that this would provide a more consistent experience for developers and for (in the case of Console) users across all the ignition projects.

It wouldn't be very difficult to make the Console code header-only if we wanted to go that route. It's probably not the most efficient design, but the Console isn't a terribly heavy feature, so I don't think it would have a noticeable impact on compilation time or code size.

The renaming isn't a critical part of the proposal. I'm mostly indifferent about it, but I'd encourage it if we were willing to do it.

[chapulina]

A few months after this issue was ticketed, the utilities component was added to ign-cmake with the suppression warnings.

3 years later, on Ignition Edifice, we added the ign-utils package to serve this purpose, so we don't stretch the responsibility of ign-cmake and it can remain focused on build utilities. The ign-utils package is already used by libraries that don't directly depend on ign-cmake, like libSDFormat.

The PIMPL pointer helper is already on ign-utils, as well as the suppression macros. See #171 about deprecating the utilities in this repository.

I'm closing this issue in favor of putting new C++ utilities into ign-utils.