pimpl IBuilder

[ryanolson]

replacing the virtual interface of IBuilder with a pimpl interface to improve abi compatibility

[cwharris]

I don't understand. This adds another level of indirection, but why? What problem is it solving that simple virtual class inheritance and a std::shared_ptr won't?

[ryanolson]

Additions to the pure virtual interface would break the abi.

These I-classes in srf/internal are the interfaces between the frontend and the backend. Probably poorly named. That which is in internal will likely be moved to it's own separate library. The goal is to make the abi of this new backend library to be as clean as possible.

[mdemoret-nv]

Before we merge any PRs related to stabilizing the ABI, I think we need to come up with guidelines potentially rename/relocate some of the I classes. It's also not very clear to me either why it's organized the way that it is. The inheritance and implementation classes seem to be all over the place

[ryanolson]

I agree we need to organize and rename them, but that shouldn't block this update. Otherwise it will just be code rot.

[mdemoret-nv]

I agree we need to organize and rename them, but that shouldn't block this update. Otherwise it will just be code rot.

The problem with this update is it's not consistent with the other PIMPL interfaces:

• srf::pipeline::Pipeline final : public internal::pipeline::IPipeline
• srf::segment::Definition final : public internal::segment::IDefinition
• srf::executor::Executor final : public internal::executor::IExecutor
• srf::segment::Builder
  • This used to inherit from internal::segment::IBuilder which was consistent

So which way should we be doing it? Without guidelines this is hard to say what it should be.

Looking at this PR in more detail, Builder seems to be different because it accepts the IBuilder backend in the constructor and combines the backend implementation with the PIMPL interface, which is very confusing. Especially since both the front end interface and IMPL classes are called Builder.

I think things would be a lot clearer if the PIMPL object was separated from the backend object so the pattern used for IBuilder was the same as IPipeline, IDefinition, etc. Maybe a IBuilderBackend object?

[ryanolson]

Builder is the unqiue one of the bunch.

Executor, Pipeline, segment::Definition are all objects that the user is expected to construct and take ownership of.

The public srf::segment::Builder is never owned by user code. It is constructed from an srf::internal::segment::Builder and only ever instantiated and used internally when the segment initializer function is evaluated.

Both pimpl and an abstract interface provide a compilation barrier so all the template headers do no propagate. the pimpl model allows for additions while the abstract interface does not.

Plus, it was a little ugly to inherit from IBuilder and also accept a reference to an IBuilder, then have builder implement the interface by forwarding to the reference. The pimpl idiom does the same thing, but it's hidden away in a compilation unit.

class Builder final : private internal::segment::IBuilder
{
    Builder(internal::segment::IBuilder& backend);

[ryanolson]

The other are all pimpl'd with a single pure virtual destructor to forbid construction without inheritance.

[drobison00]

Looks fine; one include ordering typo.

[drobison00]

I'd actually missed that the internal and public classes have the same name.

This gets really confusing, and I'd like to take a look at naming conventions, or isolation conventions between public and detail interfaces.

[mdemoret-nv]

@gpucibot merge