Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make empty constructor as peer to explicit #453

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

mjcarroll
Copy link
Contributor

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()};
};

@mjcarroll mjcarroll requested a review from caguero as a code owner October 6, 2023 15:59
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Oct 6, 2023
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

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

❗ 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%) ⬆️

... and 1 file with indirect coverage changes

@mjcarroll
Copy link
Contributor Author

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

@mjcarroll mjcarroll self-assigned this Oct 13, 2023
@mjcarroll mjcarroll marked this pull request as draft October 13, 2023 00:30
@j-rivero
Copy link
Contributor

@osrf-jenkins run tests

@j-rivero
Copy link
Contributor

@osrf-jenkins run tests please

@azeey
Copy link
Contributor

azeey commented Oct 25, 2023

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.

@mjcarroll mjcarroll marked this pull request as ready for review October 26, 2023 16:34
@mjcarroll mjcarroll requested a review from azeey November 2, 2023 15:49
@mjcarroll mjcarroll merged commit 237831a into gz-transport13 Nov 2, 2023
7 checks passed
@mjcarroll mjcarroll deleted the mjcarroll/explicit_constructor_fix branch November 2, 2023 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants