Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Feature/stl sharedptr implementation #2257

Closed
wants to merge 5 commits into from
Closed

Feature/stl sharedptr implementation #2257

wants to merge 5 commits into from

Conversation

conlain-k
Copy link
Contributor

Addresses #1493.

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Run unit tests and ATF tests. Will most likely require extensive testing due to number of files altered.

Summary

Converts instances of Utils::SharedPtr to std::shared_ptr, supported in C++11. Also uses std::make_shared. Converts non-standard functionality to use STL equivalents, cleans up lots of shared_ptr allocation issues.
Should only modify shared pointers in-place with no change in functionality. Internal API should be basically identical.

Changelog

Massive replacement of sharedPtrs
Conversion to use correct allocation processes in shared_ptr initialization
fix namespacing and remove custom implementations of non-standard functionality

NOTE:
Also modifies application_manager_impl.h to use subscribed_way_points_apps_list_.find(app->app_id()) instead of subscribed_way_points_apps_list_.find(app).

The subscribed_way_points_apps_list_ is a std::set<int32_t>, so indexing into it with a shared_ptr makes no sense. This is either poorly defined behavior or a bug. It may be the case that this is outside the scope of this PR, but the modification is required for the code to compile and the current code still passes all unit and SDL smoke tests.

Enhancements
  • STL-provided pointers are cleaner and more maintainable
Bug Fixes
  • see above note about the indexing

Tasks Remaining:

  • test aggressively using existing tests

CLA

@conlain-k
Copy link
Contributor Author

I added a commit that changed some strange naming wherein TSharedPtr got converted to Tstd::shared_ptr, resulting in an unintended extra namespacing. The change is cosmetic but improves code cleanness.

jacobkeeler
jacobkeeler requested changes Jul 17, 2018
View reviewed changes
src/components/application_manager/test/commands/command_request_impl_test.cc Outdated
@@ -65,7 +65,7 @@ using ::testing::Return;
using ::testing::SaveArg;
using ::testing::DoAll;

using ::utils::SharedPtr;
using ::std::shared_ptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the using statement can just be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops, that was a find-replace that became unneccesary. Will remove

src/components/application_manager/test/commands/hmi/close_popup_response_test.cc Outdated
@@ -48,13 +48,13 @@ namespace commands_test {
namespace hmi_commands_test {
namespace close_popup_response {

using ::utils::SharedPtr;
using ::std::shared_ptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, no reason for using

src/components/application_manager/test/commands/hmi/get_system_info_request_test.cc Outdated
@@ -49,14 +49,14 @@ namespace commands_test {
namespace hmi_commands_test {
namespace get_system_info_request {

using ::utils::SharedPtr;
using ::std::shared_ptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of these

src/components/application_manager/test/event_engine_test.cc Outdated
@@ -38,7 +38,7 @@
#include "application_manager/mock_event_observer.h"
#include "application_manager/mock_event_dispatcher.h"
#include "smart_objects/smart_object.h"
#include "utils/make_shared.h"
#include <memory>
Copy link
Contributor

Choose a reason for hiding this comment

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

Might just be able to remove

src/components/application_manager/test/policy_handler_test.cc Outdated
#include "utils/custom_string.h"
#include "policy/usage_statistics/counter.h"
#include "policy/usage_statistics/statistics_manager.h"
#include "interfaces/MOBILE_API.h"
#include "policy/mock_policy_settings.h"
#include "utils/make_shared.h"
#include <memory>
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate include, should be removed

Copy link
Contributor Author

@conlain-k conlain-k Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed Unless someone higher in the include stack pulls this in, it's needed for the shared_ptr definitions

...y_external/doc/doxygen/components/SmartObjects/Smart Objects Validation/Schema Structure.txt Outdated
@@ -57,17 +57,17 @@ Example:
//
// true, if successful
// false, if failed
TSharedPtr<ISchemaItem> success_SchemaItem = CBoolSchemaItem::create(TSchemaItemParameter<bool>());
Tshared_ptr<ISchemaItem> success_SchemaItem = CBoolSchemaItem::create(TSchemaItemParameter<bool>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this might be a find/replace error

src/components/policy/policy_regular/src/policy_manager_impl.cc Outdated
#include "policy/cache_manager.h"
#include "policy/update_status_manager.h"
#include "config_profile/profile.h"
#include "utils/timer_task_impl.h"
#include "utils/make_shared.h"
#include <memory>
Copy link
Contributor

Choose a reason for hiding this comment

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

Another duplicate include

src/components/protocol_handler/src/multiframe_builder.cc Outdated
@@ -35,7 +35,7 @@
#include <limits>

#include "utils/logger.h"
#include "utils/make_shared.h"
#include <memory>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can likely be removed

src/components/transport_manager/src/transport_manager_impl.cc Outdated
@@ -1066,7 +1066,7 @@ void TransportManagerImpl::Handle(TransportAdapterEvent event) {
LOG4CXX_ERROR(logger_, "Transport adapter failed to send data");
// TODO(YK): potential error case -> thread unsafe
// update of message content
if (event.event_data.valid()) {
if (event.event_data && event.event_data.use_count() > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason that ValidSPtr isn't used here, but is used in the USB adapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, that should be ValidSPtr

tools/InterfaceGenerator/test/generator/generators/test_expected_jsonrpc.cc Outdated
@@ -69,7 +69,7 @@ XXX::YYY::ZZZ::Test::Test()
InitFunctionSchemes(struct_schema_items, function_id_items, message_type_items);
}

TSharedPtr<ISchemaItem> XXX::YYY::ZZZ::Test::ProvideObjectSchemaItemForStruct(
Tshared_ptr<ISchemaItem> XXX::YYY::ZZZ::Test::ProvideObjectSchemaItemForStruct(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a find/replace error? I haven't found where it could be defined

@conlain-k
General cleanup, rename shared_ptr.h -> shared_ptr_helpers.h
aed926c 
Remove extra includes and usings and unintentional find-replace errors
@conlain-k conlain-k mentioned this pull request Jul 18, 2018
Merged
2 tasks
@jacobkeeler
Copy link
Contributor

duplicate of #2391

@jacobkeeler jacobkeeler deleted the feature/STL_sharedptr_implementation branch August 27, 2018 19:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jacobkeeler jacobkeeler jacobkeeler requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@conlain-k @jacobkeeler