gazebosim / gz-transport

Transport library for component communication based on publication/subscription and service calls.
https://gazebosim.org
Apache License 2.0
29 stars 36 forks source link

Make empty constructor as peer to explicit #453

Closed mjcarroll closed 8 months ago

mjcarroll commented 9 months ago

I think this is actually what we intended to do with the Node constructor.

The explicit keyword makes using the default constructor a little strange in combination with ImplPtr.

For example:

class Foo
{
   GZ_UTILS_UNIQUE_IMPL_PTR(dataPtr)
};

class Foo::Implementation
{
  gz::transport::Node node;
};

Produces the compiler error:

In file included from /usr/local/google/home/mjcarroll/workspaces/gz_ionic/src/gz-gui/src/plugins/camera_tracking/CameraTracking.cc:18:
In file included from /usr/local/google/home/mjcarroll/workspaces/gz_ionic/install/include/gz/utils2/gz/utils/ImplPtr.hh:258:
/usr/local/google/home/mjcarroll/workspaces/gz_ionic/install/include/gz/utils2/gz/utils/detail/ImplPtr.hh:145:46: error: chosen constructor is explicit in copy-initialization
            new T{std::forward<Args>(args)...},
                                             ^
/usr/local/google/home/mjcarroll/workspaces/gz_ionic/src/gz-gui/src/plugins/camera_tracking/CameraTracking.cc:434:24: note: in instantiation of function template specialization 'gz::utils::MakeUniqueImpl<gz::gui::plugins::CameraTracking::Implementation>' requested here
  : dataPtr(gz::utils::MakeUniqueImpl<Implementation>())
                       ^
/usr/local/google/home/mjcarroll/workspaces/gz_ionic/install/include/gz/transport13/gz/transport/Node.hh:203:24: note: explicit constructor declared here
      public: explicit Node(const NodeOptions &_options = NodeOptions());
                       ^
/usr/local/google/home/mjcarroll/workspaces/gz_ionic/src/gz-gui/src/plugins/camera_tracking/CameraTracking.cc:135:27: note: in implicit initialization of field 'node' with omitted initializer
  public: transport::Node node;
                          ^
1 error generated.

The other option is to update each Implementation class to explicitly pass the NodeOptions such as follows, but this seems excessive in my mind.

class Foo::Implementation
{
  gz::transport::Node node {gz::transport::NodeOptions()};
};
codecov[bot] commented 9 months ago

Codecov Report

Merging #453 (e2c0729) into gz-transport13 (b012e82) will increase coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head e2c0729 differs from pull request most recent head 257cf7c. Consider uploading reports for the commit 257cf7c to get more accurate results

@@                Coverage Diff                 @@
##           gz-transport13     #453      +/-   ##
==================================================
+ Coverage           87.78%   87.82%   +0.04%     
==================================================
  Files                  59       59              
  Lines                5696     5699       +3     
==================================================
+ Hits                 5000     5005       +5     
+ Misses                696      694       -2     
Files Coverage Δ
src/Node.cc 91.80% <100.00%> (+0.68%) :arrow_up:

... and 1 file with indirect coverage changes

mjcarroll commented 9 months ago

I'm pretty sure this is going to be problematic ABI/API-wise.

j-rivero commented 9 months ago

@osrf-jenkins run tests

j-rivero commented 9 months ago

@osrf-jenkins run tests please

azeey commented 8 months ago

I don't think this breaks ABI. My understanding is that the constructor with the default argument and the new constructor with a single argument will have the same symbol name, so this PR is essentially only adding the default constructor from an ABI perspective.