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 use of smart pointers and references #316

Merged
merged 6 commits into from
Mar 14, 2018

Conversation

shakthi-prashanth-m
Copy link
Contributor

@shakthi-prashanth-m shakthi-prashanth-m commented Mar 13, 2018

This commit replaces raw pointers by smart pointers and references as applicable.
We have to use references when we can, and pointers when we have to.

Smart pointers provide automatic resource management. In other words, they implement Resource Acquisition Is Initialization programming idiom. The main goal of this idiom is to ensure that resource acquisition occurs at the same time that the object is initialized, so that all resources for the object are created and made ready in one line of code.

std::unique_ptr is a smart pointer that owns and manages another object through a pointer and disposes of that object when the unique_ptr goes out of scope.

std::shared_ptr is a smart pointer that retains shared ownership of an object through a pointer. Several shared_ptr objects may own the same object. The object is destroyed and its memory deallocated when either of the following happens:

  • the last remaining shared_ptr owning the object is destroyed;
  • the last remaining shared_ptr owning the object is assigned another pointer via operator= or reset().

References:

Fixes #314

Shakthi Prashanth M added 3 commits March 13, 2018 12:05
Replace `Device *PluginImplBase::parent` => `Device &PluginImplBase::parent`.
And replaces everywhere as required.
Replaced wherever applicabale.
Smart pointers provide automatic resource management. In other words,
they implement "Resource Acquisition Is Initialialization programming idiom".
The main goal of this idiom is to ensure that resource acquisition occurs
at the same time that the object is initialized, so that all resources
for the object are created and made ready in one line of code.

`std::unique_ptr` is a smart pointer that owns and manages another object
through a pointer and disposes of that object when the unique_ptr goes out of scope.

Ref 1: https://docs.microsoft.com/en-us/cpp/cpp/smart-pointers-modern-cpp
Ref 2: http://en.cppreference.com/w/cpp/memory/unique_ptr
@shakthi-prashanth-m
Copy link
Contributor Author

I think, we should use make_unique construct to make them smart pointer. I shall change it.

@shakthi-prashanth-m
Copy link
Contributor Author

make_unique constructs an object of type T and wraps it in a std::unique_ptr. Its available in C++14. Its fine to use raw pointer and assign to a unique_ptr in C+11.

In `CMakeLists.txt`, include directory for example.h is added.
@@ -330,7 +330,7 @@ void DroneCoreImpl::create_device_if_not_existing(const uint8_t system_id)
}

// Create both lists in parallel
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this comment makes no sense now :) I shall remove it in next commit.

This replaces raw pointers used in connections and devices
by smart pointers. This avoids, freeing them in destructor and also
makes code clean and readable.
@shakthi-prashanth-m
Copy link
Contributor Author

Executed SitlTest.TwoConnections and SitlTest.AsyncConnect.

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from SitlTest
[ RUN      ] SitlTest.TwoConnections
Not autostarting SITL (use AUTOSTART_SITL=1)
started
[04:37:56|Info ] New device on: 127.0.0.1:14556 (udp_connection.cpp:207)
[04:37:56|Error] Remote IP unknown (udp_connection.cpp:136)
[04:37:56|Error] send fail (dronecore_impl.cpp:90)
[04:37:56|Info ] New device on: 127.0.0.1:14557 (udp_connection.cpp:207)
[04:37:57|Debug] Discovered 4294967298 (dronecore_impl.cpp:320)
[04:37:57|Debug] MAVLink: info: DISARMED by auto disarm on land (device.cpp:251)
[04:37:57|Debug] MAVLink: info: [logger] file: rootfs/fs/microsd/log/2018-03-13/1 (device.cpp:251)
[04:37:57|Debug] MAVLink: info: data link #0 lost (device.cpp:251)
[04:37:57|Debug] MAVLink: critical: First waypoint too far away: 7094 meters, 900 max (device.cpp:251)
[04:37:57|Debug] MAVLink: info: ARMED by arm/disarm component command (device.cpp:251)
[04:37:57|Debug] MAVLink: info: Using minimum takeoff altitude: 2.50 m (device.cpp:251)
[04:37:57|Debug] MAVLink: info: Takeoff detected (device.cpp:251)
[04:37:57|Debug] MAVLink: critical: Planned mission fails check. Please upload again. (device.cpp:251)
[04:37:57|Debug] MAVLink: critical: Planned mission fails check. Please upload again. (device.cpp:251)
[04:37:57|Debug] MAVLink: critical: Planned mission fails check. Please upload again. (device.cpp:251)
[04:37:57|Debug] MAVLink: critical: Planned mission fails check. Please upload again. (device.cpp:251)
[04:37:57|Debug] MAVLink: info: data link #0 regained (device.cpp:251)
[04:37:57|Debug] MAVLink: info: data link #0 regained (device.cpp:251)
found device with UUID: 4294967298
deleting it
[04:38:01|Info ] Clearing devices. (dronecore_impl.cpp:31)
[04:38:01|Info ] Destructor called. (device.cpp:43)
[04:38:01|Info ] Clearing connections. (dronecore_impl.cpp:37)
[04:38:01|Info ] UDP destruct... (udp_connection.cpp:35)
[04:38:01|Info ] Conn destruct... (connection.cpp:14)
[04:38:01|Info ] UDP destruct... (udp_connection.cpp:35)
[04:38:01|Info ] Conn destruct... (connection.cpp:14)
[04:38:02|Info ] New device on: 127.0.0.1:14557 (udp_connection.cpp:207)
[04:38:03|Debug] Discovered 4294967298 (dronecore_impl.cpp:320)
created and started yet again
[04:38:06|Info ] Clearing devices. (dronecore_impl.cpp:31)
[04:38:06|Info ] Destructor called. (device.cpp:43)
[04:38:06|Info ] Clearing connections. (dronecore_impl.cpp:37)
[04:38:06|Info ] UDP destruct... (udp_connection.cpp:35)
[04:38:06|Info ] Conn destruct... (connection.cpp:14)
exiting
[       OK ] SitlTest.TwoConnections (14016 ms)
[ RUN      ] SitlTest.AsyncConnect
Not autostarting SITL (use AUTOSTART_SITL=1)
waiting for device to appear...
[04:38:10|Info ] New device on: 127.0.0.1:14557 (udp_connection.cpp:207)
[04:38:10|Debug] MAVLink: critical: Planned mission fails check. Please upload again. (device.cpp:251)
[04:38:10|Debug] MAVLink: info: data link #1 lost (device.cpp:251)
[04:38:11|Debug] Discovered 4294967298 (dronecore_impl.cpp:320)
Found device with UUID: 4294967298
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
waiting for device to disappear...
[04:38:27|Info ] heartbeats timed out (device.cpp:256)
[04:38:27|Debug] Lost 4294967298 (dronecore_impl.cpp:328)
Lost device with UUID: 4294967298
[04:38:27|Info ] Clearing devices. (dronecore_impl.cpp:31)
[04:38:27|Info ] Destructor called. (device.cpp:43)
[04:38:27|Info ] Clearing connections. (dronecore_impl.cpp:37)
[04:38:27|Info ] UDP destruct... (udp_connection.cpp:35)
[04:38:27|Info ] Conn destruct... (connection.cpp:14)
[       OK ] SitlTest.AsyncConnect (21072 ms)
[----------] 2 tests from SitlTest (35088 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (35088 ms total)
[  PASSED  ] 2 tests.

Updates all the plugin API description about passing device object,
instead of pointer to it.
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, quite a lot of changes. Looks all correct though.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Docs changes make sense. I think you caught the required "source based" changes, but I'll check more thoroughly after this is merged. Created mavlink/MAVSDK-docs#106 to track required docs changes.

@julianoes
Copy link
Collaborator

@JonasVautherin when is a good time to merge this? 😄

@JonasVautherin
Copy link
Collaborator

I'm finishing my review now, I will merge it in a few minutes. Sorry for the delay =/

Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JonasVautherin JonasVautherin merged commit a2b9db9 into develop Mar 14, 2018
@JonasVautherin JonasVautherin deleted the prefer-reference-over-ptr branch March 14, 2018 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants