Classes not using PIMPL

[osrf-migration]

Original report (archived issue) by Shane Loretz (Bitbucket: Shane Loretz, GitHub: sloretz).

---

Follow up to today's meeting, these headers do not contain the text "dataPtr". This ticket is to add a private member std::unique_ptr<*Private> dataPtr; and forward declaration for all of them so that it is easier to fix bugs between major releases.

First I created a list with the command

find . -name "*.hh" -not -name "*TEST.hh" -not -name "*Private.hh" -exec grep --color=auto -e "dataPtr" -L "{}" "+" > /tmp/pimpl.txt

Then I manually checked and removed some files whose names suggested they wouldn't contain a class (like "Types.hh", or "Iface.hh"). I have not checked every file here.

Rendering

./rendering/JointVisual.hh
./rendering/selection_buffer/MaterialSwitcher.hh
./rendering/selection_buffer/SelectionRenderListener.hh
./rendering/FPSViewController.hh
./rendering/RFIDTagVisual.hh
./rendering/ApplyWrenchVisual.hh
./rendering/MovableText.hh
./rendering/TransmitterVisual.hh
./rendering/InertiaVisual.hh
./rendering/ArrowVisual.hh
./rendering/WrenchVisual.hh
./rendering/Road2d.hh
./rendering/ViewController.hh
./rendering/DynamicRenderable.hh
./rendering/OrbitViewController.hh
./rendering/MarkerVisual.hh
./rendering/CameraVisual.hh
./rendering/SonarVisual.hh
./rendering/LogicalCameraVisual.hh
./rendering/OriginVisual.hh
./rendering/RenderEvents.hh
./rendering/VideoVisual.hh
./rendering/LinkFrameVisual.hh
./rendering/ContactVisual.hh
./rendering/Projector.hh
./rendering/Material.hh
./rendering/COMVisual.hh
./rendering/SelectionObj.hh
./rendering/deferred_shading/DeferredLight.hh
./rendering/deferred_shading/DeferredLightCP.hh
./rendering/deferred_shading/LightMaterialGenerator.hh
./rendering/deferred_shading/GBufferSchemeHandler.hh
./rendering/deferred_shading/MaterialGenerator.hh
./rendering/deferred_shading/TechniqueDefinitions.hh
./rendering/deferred_shading/MergeMaterialGenerator.hh
./rendering/deferred_shading/MergeSchemeHandler.hh
./rendering/deferred_shading/NullSchemeHandler.hh
./rendering/deferred_shading/SSAOLogic.hh
./rendering/deferred_shading/MergeCP.hh
./rendering/deferred_shading/AmbientLight.hh
./rendering/deferred_shading/GBufferMaterialGenerator.hh
./rendering/deferred_shading/ListenerFactoryLogic.hh
./rendering/LaserVisual.hh
./rendering/RFIDVisual.hh

Common

./common/Exception.hh
./common/MeshExporter.hh
./common/ImageHeightmap.hh
./common/MeshLoader.hh
./common/Event.hh
./common/MaterialDensity.hh
./common/SkeletonAnimation.hh
./common/Console.hh
./common/MeshManager.hh
./common/Animation.hh
./common/SystemPaths.hh
./common/Mesh.hh
./common/Timer.hh
./common/Image.hh
./common/STLLoader.hh
./common/KeyEvent.hh
./common/Video.hh
./common/Skeleton.hh
./common/Time.hh
./common/HeightmapData.hh
./common/GTSMeshUtils.hh
./common/Material.hh
./common/UpdateInfo.hh
./common/BVHLoader.hh
./common/MeshCSG.hh
./common/AudioDecoder.hh
./common/KeyFrame.hh
./common/PID.hh
./common/Plugin.hh
./common/Color.hh

Physics

./physics/Contact.hh
./physics/MapShape.hh
./physics/PlaneShape.hh
./physics/HingeJoint.hh
./physics/ModelState.hh
./physics/Light.hh
./physics/Entity.hh
./physics/Model.hh
./physics/JointState.hh
./physics/Joint.hh
./physics/bullet/BulletSphereShape.hh
./physics/bullet/BulletMeshShape.hh
./physics/bullet/BulletHingeJoint.hh
./physics/bullet/BulletLink.hh
./physics/bullet/BulletUniversalJoint.hh
./physics/bullet/BulletMotionState.hh
./physics/bullet/BulletJoint.hh
./physics/bullet/BulletRayShape.hh
./physics/bullet/BulletFixedJoint.hh
./physics/bullet/BulletScrewJoint.hh
./physics/bullet/BulletMesh.hh
./physics/bullet/BulletPhysics.hh
./physics/bullet/BulletMultiRayShape.hh
./physics/bullet/gzBtUniversalConstraint.hh
./physics/bullet/BulletPlaneShape.hh
./physics/bullet/BulletPolylineShape.hh
./physics/bullet/BulletSliderJoint.hh
./physics/bullet/BulletCollision.hh
./physics/bullet/BulletHinge2Joint.hh
./physics/bullet/BulletSurfaceParams.hh
./physics/bullet/BulletBoxShape.hh
./physics/bullet/BulletCylinderShape.hh
./physics/bullet/BulletHeightmapShape.hh
./physics/bullet/BulletBallJoint.hh
./physics/ode/ODECylinderShape.hh
./physics/ode/ODEHingeJoint.hh
./physics/ode/ODEMeshShape.hh
./physics/ode/ODEFixedJoint.hh
./physics/ode/ODESliderJoint.hh
./physics/ode/ODEUniversalJoint.hh
./physics/ode/ODEPolylineShape.hh
./physics/ode/ODEMesh.hh
./physics/ode/ODEMultiRayShape.hh
./physics/ode/ODEBoxShape.hh
./physics/ode/ODEPlaneShape.hh
./physics/ode/ODEHeightmapShape.hh
./physics/ode/ODERayShape.hh
./physics/ode/ODEScrewJoint.hh
./physics/ode/ODEHinge2Joint.hh
./physics/ode/ODEBallJoint.hh
./physics/ode/ODELink.hh
./physics/ode/ODEGearboxJoint.hh
./physics/ode/ODECollision.hh
./physics/ode/ODESurfaceParams.hh
./physics/ode/ODESphereShape.hh
./physics/ode/ODEJoint.hh
./physics/Base.hh
./physics/AtmosphereFactory.hh
./physics/WorldState.hh
./physics/FixedJoint.hh
./physics/SurfaceParams.hh
./physics/RayShape.hh
./physics/State.hh
./physics/BallJoint.hh
./physics/Actor.hh
./physics/UniversalJoint.hh
./physics/JointWrench.hh
./physics/SliderJoint.hh
./physics/ContactManager.hh
./physics/BoxShape.hh
./physics/MeshShape.hh
./physics/simbody/SimbodyPolylineShape.hh
./physics/simbody/SimbodyLink.hh
./physics/simbody/SimbodyCollision.hh
./physics/simbody/SimbodyMultiRayShape.hh
./physics/simbody/SimbodyModel.hh
./physics/simbody/SimbodyCylinderShape.hh
./physics/simbody/SimbodyHingeJoint.hh
./physics/simbody/SimbodyFixedJoint.hh
./physics/simbody/SimbodyBoxShape.hh
./physics/simbody/SimbodyRayShape.hh
./physics/simbody/SimbodyMeshShape.hh
./physics/simbody/SimbodyJoint.hh
./physics/simbody/SimbodyScrewJoint.hh
./physics/simbody/SimbodyMesh.hh
./physics/simbody/SimbodyUniversalJoint.hh
./physics/simbody/SimbodyHinge2Joint.hh
./physics/simbody/SimbodyBallJoint.hh
./physics/simbody/SimbodySphereShape.hh
./physics/simbody/SimbodyPlaneShape.hh
./physics/simbody/SimbodyHeightmapShape.hh
./physics/simbody/SimbodySliderJoint.hh
./physics/simbody/SimbodyPhysics.hh
./physics/GearboxJoint.hh
./physics/PolylineShape.hh
./physics/SphereShape.hh
./physics/HeightmapShape.hh
./physics/Shape.hh
./physics/Collision.hh
./physics/Road.hh
./physics/ScrewJoint.hh
./physics/PhysicsFactory.hh
./physics/dart/DARTHingeJoint.hh
./physics/dart/DARTBallJoint.hh
./physics/dart/DARTSliderJoint.hh
./physics/dart/DARTFixedJoint.hh
./physics/dart/DARTUniversalJoint.hh
./physics/dart/DARTHinge2Joint.hh
./physics/dart/DARTScrewJoint.hh
./physics/CollisionState.hh
./physics/Link.hh
./physics/PhysicsEngine.hh
./physics/MultiRayShape.hh
./physics/CylinderShape.hh
./physics/LightState.hh
./physics/Hinge2Joint.hh
./physics/LinkState.hh

Transport

./transport/SubscribeOptions.hh
./transport/IOManager.hh
./transport/PublicationTransport.hh
./transport/Publication.hh
./transport/TransportTypes.hh
./transport/Node.hh
./transport/CallbackHelper.hh
./transport/Subscriber.hh
./transport/Connection.hh
./transport/Publisher.hh
./transport/ConnectionManager.hh
./transport/TopicManager.hh
./transport/SubscriptionTransport.hh

Sensors

./sensors/WirelessTransceiver.hh
./sensors/SensorManager.hh
./sensors/GaussianNoiseModel.hh
./sensors/Noise.hh
./sensors/SensorFactory.hh

GUI

./gui/Actions.hh
./gui/GuiPlugin.hh
./gui/MouseEventHandler.hh
./gui/OculusWindow.hh
./gui/model/GraphScene.hh
./gui/model/GraphView.hh
./gui/model/ModelEditorEvents.hh
./gui/model/ModelTreeWidget.hh
./gui/model/EditorMaterialSwitcher.hh
./gui/model/LinkConfig.hh
./gui/model/VisualConfig.hh
./gui/model/ImportDialog.hh
./gui/model/SchematicViewWidget.hh
./gui/model/CollisionConfig.hh
./gui/model/ModelData.hh
./gui/TopicSelector.hh
./gui/ModelRightMenu.hh
./gui/RenderWidget.hh
./gui/ToolsWidget.hh
./gui/viewers/TopicView.hh
./gui/viewers/ViewFactory.hh
./gui/viewers/TextView.hh
./gui/viewers/LaserView.hh
./gui/Editor.hh
./gui/building/BuildingEditor.hh
./gui/building/EditorView.hh
./gui/EntityMaker.hh

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

There are a few false positives on this list. For example, the derived *Visual classes are using private classes inherited from protected: VisualPrivate *dataPtr

[osrf-migration]

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

---

I wanted to make a suggestion, because I think it could allow a much cleaner diff and save a lot of work. We could define some macros:

#ifdef __MSC_VER

#define INTERNAL_GAZEBO_IGNORE_DLL_EXPORT \
  __pragma(warning(push)) \
  __pragma(warning(disable: 4251))

#define INTERNAL_GAZEBO_RESUME_DLL_EXPORT \
  __pragma(warning(pop))

#else

#define INTERNAL_GAZEBO_IGNORE_DLL_EXPORT

#define INTERNAL_GAZEBO_RESUME_DLL_EXPORT

#endif

#define INTERNAL_GAZEBO_DATAPTR \
  class Implementation; \
  INTERNAL_GAZEBO_IGNORE_DLL_EXPORT \
  std::unique_ptr<Implementation> dataPtr; \
  INTERNAL_GAZEBO_RESUME_DLL_EXPORT

Then we can just put a single INTERNAL_GAZEBO_DATAPTR in each class that violates PIMPL to forward declare the implementation class, add the dataPtr field, and suppress the DLL interface warnings that will be emitted by Visual Studio.

To define the implementation class, in a source file we can put:

class Xxx::Implementation
{
  // Data fields of implementation class go here
};

where Xxx is the name of the class that is being PIMPLed.

(For the record, I'm writing all of this from my phone in the airport, so I apologize in advance for any mistakes.)

[osrf-migration]

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

---

Side note: we'll probably also have to add explicit destructors for each class that we PIMPLize, so those destructors would have to be added alongside the macro for any classes that don't already have one. We would need to do that with or without the macro approach that I proposed.