From 3b12276bc33f99bc721b4c0ffe1f967a41fc59b7 Mon Sep 17 00:00:00 2001 From: Oblarg Date: Sat, 3 Aug 2019 02:47:17 -0400 Subject: [PATCH] SendableBase: remove unnecessary synchronization (#1797) Also fixes the move constructor to update LiveWindow to follow the move. --- .../main/native/cpp/livewindow/LiveWindow.cpp | 4 +- .../cpp/smartdashboard/SendableBase.cpp | 42 +++++++++---------- .../include/frc/livewindow/LiveWindow.h | 3 +- .../include/frc/smartdashboard/SendableBase.h | 11 +++-- .../edu/wpi/first/wpilibj/SendableBase.java | 10 ++--- 5 files changed, 33 insertions(+), 37 deletions(-) diff --git a/wpilibc/src/main/native/cpp/livewindow/LiveWindow.cpp b/wpilibc/src/main/native/cpp/livewindow/LiveWindow.cpp index 7a4f1a38c7..51688f3873 100644 --- a/wpilibc/src/main/native/cpp/livewindow/LiveWindow.cpp +++ b/wpilibc/src/main/native/cpp/livewindow/LiveWindow.cpp @@ -81,9 +81,9 @@ void LiveWindow::AddChild(Sendable* parent, void* child) { comp.telemetryEnabled = false; } -void LiveWindow::Remove(Sendable* sendable) { +bool LiveWindow::Remove(Sendable* sendable) { std::scoped_lock lock(m_impl->mutex); - m_impl->components.erase(sendable); + return m_impl->components.erase(sendable); } void LiveWindow::EnableTelemetry(Sendable* sendable) { diff --git a/wpilibc/src/main/native/cpp/smartdashboard/SendableBase.cpp b/wpilibc/src/main/native/cpp/smartdashboard/SendableBase.cpp index 4110b1fdfd..96f054554c 100644 --- a/wpilibc/src/main/native/cpp/smartdashboard/SendableBase.cpp +++ b/wpilibc/src/main/native/cpp/smartdashboard/SendableBase.cpp @@ -17,39 +17,35 @@ SendableBase::SendableBase(bool addLiveWindow) { if (addLiveWindow) LiveWindow::GetInstance()->Add(this); } -SendableBase::~SendableBase() { LiveWindow::GetInstance()->Remove(this); } - -SendableBase::SendableBase(SendableBase&& rhs) { - m_name = std::move(rhs.m_name); - m_subsystem = std::move(rhs.m_subsystem); +SendableBase::SendableBase(SendableBase&& other) + : m_name(std::move(other.m_name)), + m_subsystem(std::move(other.m_subsystem)) { + auto&& lw = LiveWindow::GetInstance(); + if (lw->Remove(&other)) { + lw->Add(this); + } } -SendableBase& SendableBase::operator=(SendableBase&& rhs) { - Sendable::operator=(std::move(rhs)); - - m_name = std::move(rhs.m_name); - m_subsystem = std::move(rhs.m_subsystem); +SendableBase& SendableBase::operator=(SendableBase&& other) { + m_name = std::move(other.m_name); + m_subsystem = std::move(other.m_subsystem); + auto&& lw = LiveWindow::GetInstance(); + if (lw->Remove(&other)) { + lw->Add(this); + } return *this; } -std::string SendableBase::GetName() const { - std::scoped_lock lock(m_mutex); - return m_name; -} +SendableBase::~SendableBase() { LiveWindow::GetInstance()->Remove(this); } -void SendableBase::SetName(const wpi::Twine& name) { - std::scoped_lock lock(m_mutex); - m_name = name.str(); -} +std::string SendableBase::GetName() const { return m_name; } -std::string SendableBase::GetSubsystem() const { - std::scoped_lock lock(m_mutex); - return m_subsystem; -} +void SendableBase::SetName(const wpi::Twine& name) { m_name = name.str(); } + +std::string SendableBase::GetSubsystem() const { return m_subsystem; } void SendableBase::SetSubsystem(const wpi::Twine& subsystem) { - std::scoped_lock lock(m_mutex); m_subsystem = subsystem.str(); } diff --git a/wpilibc/src/main/native/include/frc/livewindow/LiveWindow.h b/wpilibc/src/main/native/include/frc/livewindow/LiveWindow.h index 0421105f1d..575cb0c22d 100644 --- a/wpilibc/src/main/native/include/frc/livewindow/LiveWindow.h +++ b/wpilibc/src/main/native/include/frc/livewindow/LiveWindow.h @@ -67,8 +67,9 @@ class LiveWindow { * Remove the component from the LiveWindow. * * @param sendable component to remove + * @return true if the component was removed; false if it was not present */ - void Remove(Sendable* component); + bool Remove(Sendable* component); /** * Enable telemetry for a single component. diff --git a/wpilibc/src/main/native/include/frc/smartdashboard/SendableBase.h b/wpilibc/src/main/native/include/frc/smartdashboard/SendableBase.h index 6d2fbbee96..d2f0091eb6 100644 --- a/wpilibc/src/main/native/include/frc/smartdashboard/SendableBase.h +++ b/wpilibc/src/main/native/include/frc/smartdashboard/SendableBase.h @@ -1,5 +1,5 @@ /*----------------------------------------------------------------------------*/ -/* Copyright (c) 2017-2018 FIRST. All Rights Reserved. */ +/* Copyright (c) 2017-2019 FIRST. All Rights Reserved. */ /* Open Source Software - may be modified and shared by FRC teams. The code */ /* must be accompanied by the FIRST BSD license file in the root directory of */ /* the project. */ @@ -10,8 +10,6 @@ #include #include -#include - #include "frc/smartdashboard/Sendable.h" namespace frc { @@ -27,8 +25,10 @@ class SendableBase : public Sendable { ~SendableBase() override; - SendableBase(SendableBase&& rhs); - SendableBase& operator=(SendableBase&& rhs); + SendableBase(const SendableBase&) = default; + SendableBase& operator=(const SendableBase&) = default; + SendableBase(SendableBase&&); + SendableBase& operator=(SendableBase&&); using Sendable::SetName; @@ -73,7 +73,6 @@ class SendableBase : public Sendable { void SetName(const wpi::Twine& moduleType, int moduleNumber, int channel); private: - mutable wpi::mutex m_mutex; std::string m_name; std::string m_subsystem = "Ungrouped"; }; diff --git a/wpilibj/src/main/java/edu/wpi/first/wpilibj/SendableBase.java b/wpilibj/src/main/java/edu/wpi/first/wpilibj/SendableBase.java index b663ed52d3..b302a931bb 100644 --- a/wpilibj/src/main/java/edu/wpi/first/wpilibj/SendableBase.java +++ b/wpilibj/src/main/java/edu/wpi/first/wpilibj/SendableBase.java @@ -1,5 +1,5 @@ /*----------------------------------------------------------------------------*/ -/* Copyright (c) 2017-2018 FIRST. All Rights Reserved. */ +/* Copyright (c) 2017-2019 FIRST. All Rights Reserved. */ /* Open Source Software - may be modified and shared by FRC teams. The code */ /* must be accompanied by the FIRST BSD license file in the root directory of */ /* the project. */ @@ -46,12 +46,12 @@ public abstract class SendableBase implements Sendable, AutoCloseable { } @Override - public final synchronized String getName() { + public final String getName() { return m_name; } @Override - public final synchronized void setName(String name) { + public final void setName(String name) { m_name = name; } @@ -77,12 +77,12 @@ public abstract class SendableBase implements Sendable, AutoCloseable { } @Override - public final synchronized String getSubsystem() { + public final String getSubsystem() { return m_subsystem; } @Override - public final synchronized void setSubsystem(String subsystem) { + public final void setSubsystem(String subsystem) { m_subsystem = subsystem; }