tango-controls/cppTango

Public/private header separation and stable ABI proposal

Closed this issue · 0 comments

mliszcz commented

I recently started looking into what can be done not to expose all system internals in tango.h.

The goals are to:

  • define stable API and ABI for Tango which is not impacted by internal refactoring and bugfixes
  • have the same header layout in source tree as when installed on production system (to prevent issues like in #720)
  • improve compilation times
    • currently any change in most of the header files requires recompilation of all translation units because everything is transitively included from tango.h
    • there should be no need for hacks like PCHs if #includes were used correctly
  • not expose libraries used internally by Tango
    • this will allow to include boost (or whatever) without forcing users to install it
    • after breaking API a bit, it should be possible to hide omniORB and ZMQ as well (allows to have the same API for Tango v10)

It looks like a lot of work but it is really mostly renaming and moving stuff around, which can be automated to some extent with refactoring tools and IDEs. I also do not expect to introduce much conflicts with pending PRs because we do not rename files and we change mostly function declaration lines (like Attribute::do_something to AttributePrivate::do_something).

The basic idea is to change the classes we modify frequently to use PImpl idiom (a bit modified to support inheritance). Here is a blog-post describing how QT (much larger than Tango) is doing this: https://wiki.qt.io/D-Pointer .

My proposal is to have something similar in Tango. We may proceed as follows:

  1. PImpl all relevant classes, the procedure is like:
    • Rename Attribute to AttributePrivate
    • Create Attribute class in <repo>/include/tango/attribute.h
      • stores owning pointer to AttributePrivate
      • has only methods which are currently defined in @publicsection of Attribute
      • forwards all calls to AttributePrivate
      • has no protected data members (users do not inherit from this class anyway)
      • has additional protected constructor taking AttributePrivate (to support WAttribute inheritance)
    • Rename WAttribute to WAttributePrivate (WAttributePrivate inherits from AttributePrivate)
    • Create WAttribute inheriting from Attribute
      • in constructor, creates WAttributePrivate and passes it to Attribute using special constructor
    • Change MultiAttribute to store Attribute instead of AttributePrivate
    • Pass Attribute/WAtttribute again to device's read/write callbacks
    • Proceed in similar way with DeviceImpl, Device_5Impl, ...
      • Except that DeviceImpl does not inherit from any class (even CORBA servant), but wraps DeviceImplPrivate which inherits from the servant
      • DeviceImpl has some protected members visible to the users. If this is intentional, they must stay here and DeviceImplPrivate needs 'backpointer' to DeviceImpl.
    • Change Command and Pipe classes in similar way
    • Change Util class in similar way
    • Change Connection and DeviceProxy classes in similar way (although I have not looked much into client side)
  2. Leave simple classes like Attr, AttrProp as they are, just move them to /include/tango/
    • Headers in /include/tango/ should only include other headers from this directory and include idl/tango.h.
      Including anything from cppapi is prohibited here.
  3. Change CMake to install only /include/tango directory. It should contain everything (and nothing more) device servers need to compile.
  4. Remove cppapi/server/tango.h, change all files in cppapi to include only what they use, remove PCHs
  5. At this point no ZMQ and very little of CORBA should be visible in public headers. We may gradually remove them.
    • We deprecate calls like ApiUtil::get_orb, ApiUtil::get_poa, maybe return nullptr or throw an exception
    • Add extra overloads to Command class to work with std::any instead of CORBA::Any.
    • Maybe we can add dummy classes like AttributeValue with the same binary layout as classes generated from IDL and start using them, hiding any CORBA headers from the users.

After all this is done, we will have following directory structure:

/include/
  tango/
    tango.h (includes everything from current directory using quotes "")
    attribute.h (Attribute class)
    ...
/cppapi/
  /server/
    attribute.h (AttributePrivate class)
  ...

When building cppTango, include paths are: SRC/include/, SRC/cppapi/server, SRC/cppapi/client. We may assume that changing anything in SRC/include breaks ABI / API.

All this can be introduced gradually, with multiple PRs.

If we see benefit in doing this, I can work on a PR. Let me know what you think.