During Broker refactoring, I noticed the following: all headers in
broker/* include either standard library headers or Broker headers. This
appears to be by design, which makes sense to me.
As a library writer, one faces the tricky question of exposing headers
from dependencies. For example, Broker currently has it's own
broker::util::optional, which ships as a (now outdated) copy of the
corresponding CAF source. I am inclined to change this copy to an
include that points directly into CAF headers, with the following
rationale: Broker already depends on CAF, and a system that has CAF
installed always ships with CAF headers. (Strictly speaking, we're not
copying the code of <vector> into broker either, but relying on it via
an include.)
From a user perspective, nothing changes here. A user will never include
a CAF header, but may rely on it during compilation. Here's what I an
example of what I want broker/util/optional.hh to look like:
#include "caf/optional.hpp" // <--- New include.
namespace broker { namespace util {
using caf::optional;
}}
Currently we have:
// Note the absence of a CAF include.
namespace broker { namespace util {
template <class T>
class optional {
// code copied from CAF
};
}}
Relying on the former form is more maintainable, and allows us to stay
in sync with upstream fixes and improvements on the CAF side. I'm
checking in here on the list to see whether anyone has objections.
Just thought I’d share the logic behind the original decision. It was by design not to expose any CAF features directly in the public API of Broker, sort of as a proof that Broker’s interface would be sound enough to still work with arbitrary messaging back-ends/libraries besides CAF — i.e. treat the use of CAF an implementation detail.
Though, now I think this a case where including a CAF header might be an improvement and doesn’t defeat the original intention of treating CAF like an implementation detail since “optional” types aren’t a CAF-specific concept to begin with. The problem was just that they don’t come standard until c++17, I needed to get them some place, but the arbitrary rule I had for myself at the time said to generally not include CAF things in Broker’s public API. At the time, the risk of a copied version getting outdated seemed a lower priority to me than keeping Broker’s interface/design more simple/coherent in my head.
I think this is the only instance of this type of situation popping up when I was working on Broker and I can’t recall any other reasoning that led me handle it that way, so you’re proposed change looks good to me. Hope the explanation helps.
[..] i.e. treat the use of CAF an implementation detail.
This is the clean way to think about layering and creating abstractions.
It applies to the API perspective, though. As long as CAF internals are
hidden from a Broker user, we are good.
The "implementation detail" maxim lead to artifacts like PIMPL. This
certainly made sense at the time where we considered multiple messaging
backends. At this point, we are invested into CAF, and I don't think
switching will happen anytime soon. Therefore, I don't think we need to
keep up the implementation-hiding abstractions, such as PIMPL, which
come at the cost of development productivity and performance (they are
essentially a compiler firewall due to type erasure).
Moving forward, I plan to remove the PIMPL design while keeping CAF
hidden from the Broker API, but we'll see more CAF code in Broker
headers. That's fine in my thinking, because anyone developing and
compiling a Broker application must have CAF installed anyway.
At the time, the risk of a copied version getting outdated seemed a
lower priority to me than keeping Broker’s interface/design more
simple/coherent in my head.
And to be clear: that rationale totally makes sense in this context and
at the time of writing.
A possible benefit to PIMPL is improving ability to maintain binary compatibility across releases (i.e. programs dynamically linked to Broker don’t need a recompile if a new release is binary compatible). Providing stable-ish ABIs seems like something libraries often do, so I tried to plan that in to Broker. Don’t know if I did that well, or there’s better strategies to use, or I was the only one worried about that to begin with, but thought I’d mention it just in case it wasn’t even on your radar.
Providing stable-ish ABIs seems like something libraries often do, so
I tried to plan that in to Broker. Don’t know if I did that well, or
there’s better strategies to use, or I was the only one worried about
that to begin with, but thought I’d mention it just in case it wasn’t
even on your radar.
Indeed, it wasn't on my radar that you employed PIMPL to achieve ABI
compatibility.
At this point, I'm inclined to move towards a more light-weight model
that is less robust against ABI changes. I believe we still need more
experience with the API. Once the API matures, hiding central
implementation aspects to increase ABI stability becomes the next
priority to improve medium- to long-term release compatibility.