From c929d2509530b0262681c8e6619609f44bfceea4 Mon Sep 17 00:00:00 2001 From: Tim Kilbourn Date: Wed, 29 Apr 2015 13:50:17 -0700 Subject: [PATCH] Clarify usage of smart pointers - Members should be smart (shared or unique) - Prefer function args to be bare, unless the arg is intended to be stored by the callee - Function args that are smart should be const refs to avoid an extra copy Change-Id: I8052fa432bcffbabff9d67a8d568640cac64d4ad --- modules/input/evdev/EvdevModule.cpp | 8 ++++---- modules/input/evdev/InputDevice.cpp | 2 +- modules/input/evdev/InputDevice.h | 2 +- modules/input/evdev/InputDeviceManager.cpp | 6 +++--- modules/input/evdev/InputDeviceManager.h | 6 +++--- modules/input/evdev/InputHub.cpp | 29 +++++++++++++---------------- modules/input/evdev/InputHub.h | 12 ++++++------ tests/input/evdev/InputHub_test.cpp | 27 ++++++++++++++------------- 8 files changed, 45 insertions(+), 47 deletions(-) diff --git a/modules/input/evdev/EvdevModule.cpp b/modules/input/evdev/EvdevModule.cpp index e9c8222..f6df219 100644 --- a/modules/input/evdev/EvdevModule.cpp +++ b/modules/input/evdev/EvdevModule.cpp @@ -47,16 +47,16 @@ private: InputHost mInputHost; std::shared_ptr mDeviceManager; - std::shared_ptr mInputHub; + std::unique_ptr mInputHub; std::thread mPollThread; }; -static std::shared_ptr gEvdevModule; +static std::unique_ptr gEvdevModule; EvdevModule::EvdevModule(InputHost inputHost) : mInputHost(inputHost), mDeviceManager(std::make_shared()), - mInputHub(std::make_shared(mDeviceManager)) {} + mInputHub(std::make_unique(mDeviceManager)) {} void EvdevModule::init() { ALOGV("%s", __func__); @@ -98,7 +98,7 @@ static void input_init(const input_module_t* module, input_host_t* host, input_host_callbacks_t cb) { LOG_ALWAYS_FATAL_IF(strcmp(module->common.id, INPUT_HARDWARE_MODULE_ID) != 0); InputHost inputHost = {host, cb}; - gEvdevModule = std::make_shared(inputHost); + gEvdevModule = std::make_unique(inputHost); gEvdevModule->init(); } diff --git a/modules/input/evdev/InputDevice.cpp b/modules/input/evdev/InputDevice.cpp index c0b59d7..16f8039 100644 --- a/modules/input/evdev/InputDevice.cpp +++ b/modules/input/evdev/InputDevice.cpp @@ -34,7 +34,7 @@ namespace android { -EvdevDevice::EvdevDevice(std::shared_ptr node) : +EvdevDevice::EvdevDevice(const std::shared_ptr& node) : mDeviceNode(node) {} void EvdevDevice::processInput(InputEvent& event, nsecs_t currentTime) { diff --git a/modules/input/evdev/InputDevice.h b/modules/input/evdev/InputDevice.h index 3aa16cc..7a99f90 100644 --- a/modules/input/evdev/InputDevice.h +++ b/modules/input/evdev/InputDevice.h @@ -43,7 +43,7 @@ protected: */ class EvdevDevice : public InputDeviceInterface { public: - explicit EvdevDevice(std::shared_ptr node); + explicit EvdevDevice(const std::shared_ptr& node); virtual ~EvdevDevice() override = default; virtual void processInput(InputEvent& event, nsecs_t currentTime) override; diff --git a/modules/input/evdev/InputDeviceManager.cpp b/modules/input/evdev/InputDeviceManager.cpp index ceddd90..79a9610 100644 --- a/modules/input/evdev/InputDeviceManager.cpp +++ b/modules/input/evdev/InputDeviceManager.cpp @@ -24,7 +24,7 @@ namespace android { -void InputDeviceManager::onInputEvent(std::shared_ptr node, InputEvent& event, +void InputDeviceManager::onInputEvent(const std::shared_ptr& node, InputEvent& event, nsecs_t event_time) { if (mDevices[node] == nullptr) { ALOGE("got input event for unknown node %s", node->getPath().c_str()); @@ -33,11 +33,11 @@ void InputDeviceManager::onInputEvent(std::shared_ptr node, Inp mDevices[node]->processInput(event, event_time); } -void InputDeviceManager::onDeviceAdded(std::shared_ptr node) { +void InputDeviceManager::onDeviceAdded(const std::shared_ptr& node) { mDevices[node] = std::make_shared(node); } -void InputDeviceManager::onDeviceRemoved(std::shared_ptr node) { +void InputDeviceManager::onDeviceRemoved(const std::shared_ptr& node) { if (mDevices[node] == nullptr) { ALOGE("could not remove unknown node %s", node->getPath().c_str()); return; diff --git a/modules/input/evdev/InputDeviceManager.h b/modules/input/evdev/InputDeviceManager.h index b652155..2c0ffc8 100644 --- a/modules/input/evdev/InputDeviceManager.h +++ b/modules/input/evdev/InputDeviceManager.h @@ -36,10 +36,10 @@ class InputDeviceManager : public InputCallbackInterface { public: virtual ~InputDeviceManager() override = default; - virtual void onInputEvent(std::shared_ptr node, InputEvent& event, + virtual void onInputEvent(const std::shared_ptr& node, InputEvent& event, nsecs_t event_time) override; - virtual void onDeviceAdded(std::shared_ptr node) override; - virtual void onDeviceRemoved(std::shared_ptr node) override; + virtual void onDeviceAdded(const std::shared_ptr& node) override; + virtual void onDeviceRemoved(const std::shared_ptr& node) override; private: template diff --git a/modules/input/evdev/InputHub.cpp b/modules/input/evdev/InputHub.cpp index e72ac2e..ee64b29 100644 --- a/modules/input/evdev/InputHub.cpp +++ b/modules/input/evdev/InputHub.cpp @@ -396,7 +396,7 @@ void EvdevDeviceNode::disableDriverKeyRepeat() { } } -InputHub::InputHub(std::shared_ptr cb) : +InputHub::InputHub(const std::shared_ptr& cb) : mInputCallback(cb) { // Determine the type of suspend blocking we can do on this device. There // are 3 options, in decreasing order of preference: @@ -670,9 +670,8 @@ status_t InputHub::readNotify() { ALOGV("inotify event for path %s", path.c_str()); if (event->mask & IN_CREATE) { - std::shared_ptr deviceNode; - status_t res = openNode(path, &deviceNode); - if (res != OK) { + auto deviceNode = openNode(path); + if (deviceNode == nullptr) { ALOGE("could not open device node %s. err=%d", path.c_str(), res); } else { mInputCallback->onDeviceAdded(deviceNode); @@ -680,7 +679,7 @@ status_t InputHub::readNotify() { } else { auto deviceNode = findNodeByPath(path); if (deviceNode != nullptr) { - status_t ret = closeNode(deviceNode); + status_t ret = closeNode(deviceNode.get()); if (ret != OK) { ALOGW("Could not close device %s. errno=%d", path.c_str(), ret); } else { @@ -712,8 +711,8 @@ status_t InputHub::scanDir(const std::string& path) { continue; } std::string filename = path + "/" + dirent->d_name; - std::shared_ptr node; - if (openNode(filename, &node) != OK) { + auto node = openNode(filename); + if (node == nullptr) { ALOGE("could not open device node %s", filename.c_str()); } else { mInputCallback->onDeviceAdded(node); @@ -723,18 +722,16 @@ status_t InputHub::scanDir(const std::string& path) { return OK; } -status_t InputHub::openNode(const std::string& path, - std::shared_ptr* outNode) { +std::shared_ptr InputHub::openNode(const std::string& path) { ALOGV("opening %s...", path.c_str()); auto evdevNode = std::shared_ptr(EvdevDeviceNode::openDeviceNode(path)); if (evdevNode == nullptr) { - return UNKNOWN_ERROR; + return nullptr; } auto fd = evdevNode->getFd(); ALOGV("opened %s with fd %d", path.c_str(), fd); - *outNode = std::static_pointer_cast(evdevNode); - mDeviceNodes[fd] = *outNode; + mDeviceNodes[fd] = evdevNode; struct epoll_event eventItem{}; eventItem.events = EPOLLIN; if (mWakeupMechanism == WakeMechanism::EPOLL_WAKEUP) { @@ -743,7 +740,7 @@ status_t InputHub::openNode(const std::string& path, eventItem.data.u32 = fd; if (epoll_ctl(mEpollFd, EPOLL_CTL_ADD, fd, &eventItem)) { ALOGE("Could not add device fd to epoll instance. errno=%d", errno); - return -errno; + return nullptr; } if (mNeedToCheckSuspendBlockIoctl) { @@ -765,12 +762,12 @@ status_t InputHub::openNode(const std::string& path, mNeedToCheckSuspendBlockIoctl = false; } - return OK; + return evdevNode; } -status_t InputHub::closeNode(const std::shared_ptr& node) { +status_t InputHub::closeNode(const InputDeviceNode* node) { for (auto pair : mDeviceNodes) { - if (pair.second.get() == node.get()) { + if (pair.second.get() == node) { return closeNodeByFd(pair.first); } } diff --git a/modules/input/evdev/InputHub.h b/modules/input/evdev/InputHub.h index bec327a..dfab3db 100644 --- a/modules/input/evdev/InputHub.h +++ b/modules/input/evdev/InputHub.h @@ -89,10 +89,10 @@ protected: /** Callback interface for receiving input events, including device changes. */ class InputCallbackInterface { public: - virtual void onInputEvent(std::shared_ptr node, InputEvent& event, + virtual void onInputEvent(const std::shared_ptr& node, InputEvent& event, nsecs_t event_time) = 0; - virtual void onDeviceAdded(std::shared_ptr node) = 0; - virtual void onDeviceRemoved(std::shared_ptr node) = 0; + virtual void onDeviceAdded(const std::shared_ptr& node) = 0; + virtual void onDeviceRemoved(const std::shared_ptr& node) = 0; protected: InputCallbackInterface() = default; @@ -129,7 +129,7 @@ protected: */ class InputHub : public InputHubInterface { public: - explicit InputHub(std::shared_ptr cb); + explicit InputHub(const std::shared_ptr& cb); virtual ~InputHub() override; virtual status_t registerDevicePath(const std::string& path) override; @@ -143,8 +143,8 @@ public: private: status_t readNotify(); status_t scanDir(const std::string& path); - status_t openNode(const std::string& path, std::shared_ptr* outNode); - status_t closeNode(const std::shared_ptr& node); + std::shared_ptr openNode(const std::string& path); + status_t closeNode(const InputDeviceNode* node); status_t closeNodeByFd(int fd); std::shared_ptr findNodeByPath(const std::string& path); diff --git a/tests/input/evdev/InputHub_test.cpp b/tests/input/evdev/InputHub_test.cpp index f2c8edf..f2967b9 100644 --- a/tests/input/evdev/InputHub_test.cpp +++ b/tests/input/evdev/InputHub_test.cpp @@ -41,11 +41,11 @@ namespace tests { using namespace std::literals::chrono_literals; -using InputCbFunc = std::function, InputEvent&, nsecs_t)>; -using DeviceCbFunc = std::function)>; +using InputCbFunc = std::function&, InputEvent&, nsecs_t)>; +using DeviceCbFunc = std::function&)>; -static const InputCbFunc kNoopInputCb = [](std::shared_ptr, InputEvent&, nsecs_t){}; -static const DeviceCbFunc kNoopDeviceCb = [](std::shared_ptr){}; +static const InputCbFunc kNoopInputCb = [](const std::shared_ptr&, InputEvent&, nsecs_t){}; +static const DeviceCbFunc kNoopDeviceCb = [](const std::shared_ptr&){}; class TestInputCallback : public InputCallbackInterface { public: @@ -57,14 +57,14 @@ public: void setDeviceAddedCallback(DeviceCbFunc cb) { mDeviceAddedCb = cb; } void setDeviceRemovedCallback(DeviceCbFunc cb) { mDeviceRemovedCb = cb; } - virtual void onInputEvent(std::shared_ptr node, InputEvent& event, + virtual void onInputEvent(const std::shared_ptr& node, InputEvent& event, nsecs_t event_time) override { mInputCb(node, event, event_time); } - virtual void onDeviceAdded(std::shared_ptr node) override { + virtual void onDeviceAdded(const std::shared_ptr& node) override { mDeviceAddedCb(node); } - virtual void onDeviceRemoved(std::shared_ptr node) override { + virtual void onDeviceRemoved(const std::shared_ptr& node) override { mDeviceRemovedCb(node); } @@ -101,7 +101,7 @@ TEST_F(InputHubTest, DISABLED_testDeviceAdded) { std::string pathname; // Expect that this callback will run and set handle and pathname. mCallback->setDeviceAddedCallback( - [&](std::shared_ptr node) { + [&](const std::shared_ptr& node) { pathname = node->getPath(); }); @@ -136,11 +136,11 @@ TEST_F(InputHubTest, DISABLED_testDeviceRemoved) { std::shared_ptr tempNode; // Expect that these callbacks will run for the above device file. mCallback->setDeviceAddedCallback( - [&](std::shared_ptr node) { + [&](const std::shared_ptr& node) { tempNode = node; }); mCallback->setDeviceRemovedCallback( - [&](std::shared_ptr node) { + [&](const std::shared_ptr& node) { EXPECT_EQ(tempNode, node); }); @@ -182,7 +182,8 @@ TEST_F(InputHubTest, DISABLED_testInputEvent) { // Expect this callback to run when the input event is read. nsecs_t expectedWhen = systemTime(CLOCK_MONOTONIC) + ms2ns(inputDelayMs.count()); mCallback->setInputCallback( - [&](std::shared_ptr node, InputEvent& event, nsecs_t event_time) { + [&](const std::shared_ptr& node, InputEvent& event, + nsecs_t event_time) { EXPECT_NEAR(expectedWhen, event_time, ms2ns(TIMING_TOLERANCE_MS)); EXPECT_EQ(s2ns(1), event.when); EXPECT_EQ(tempFileName, node->getPath()); @@ -211,7 +212,7 @@ TEST_F(InputHubTest, DISABLED_testCallbackOrder) { // Setup the callback for input events. Should run before the device // callback. mCallback->setInputCallback( - [&](std::shared_ptr, InputEvent&, nsecs_t) { + [&](const std::shared_ptr&, InputEvent&, nsecs_t) { ASSERT_FALSE(deviceCallbackFinished); inputCallbackFinished = true; }); @@ -219,7 +220,7 @@ TEST_F(InputHubTest, DISABLED_testCallbackOrder) { // Setup the callback for device removal. Should run after the input // callback. mCallback->setDeviceRemovedCallback( - [&](std::shared_ptr node) { + [&](const std::shared_ptr& node) { ASSERT_TRUE(inputCallbackFinished) << "input callback did not run before device changed callback"; // Make sure the correct device was removed. -- 2.11.0