| Message ID | 20210416103141.1483745-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for adding this function. On Fri, 16 Apr 2021 at 11:31, Naushir Patuck <naush@raspberrypi.com> wrote: > > Add the move operator implementation for the RPiController::Metadata > class. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/metadata.hpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp > index 4f44ffc6771c..c55194040f2f 100644 > --- a/src/ipa/raspberrypi/controller/metadata.hpp > +++ b/src/ipa/raspberrypi/controller/metadata.hpp > @@ -45,6 +45,14 @@ public: > data_ = other.data_; > return *this; > } > + Metadata &operator=(Metadata &&other) > + { > + std::lock_guard<std::mutex> lock(mutex_); > + std::lock_guard<std::mutex> other_lock(other.mutex_); > + data_ = std::move(other.data_); > + other.data_.clear(); I did wonder slightly whether you need the explicit "clear" here, but I guess you probably do - it's left in a "valid but unspecified" state (even though in practice that would normally turn out to be empty). So: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > + return *this; > + } > template<typename T> T *GetLocked(std::string const &tag) > { > // This allows in-place access to the Metadata contents, > -- > 2.25.1 >
Hi Naush, Thank you for the patch. On Fri, Apr 16, 2021 at 11:31:40AM +0100, Naushir Patuck wrote: > Add the move operator implementation for the RPiController::Metadata > class. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/metadata.hpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp > index 4f44ffc6771c..c55194040f2f 100644 > --- a/src/ipa/raspberrypi/controller/metadata.hpp > +++ b/src/ipa/raspberrypi/controller/metadata.hpp > @@ -45,6 +45,14 @@ public: > data_ = other.data_; > return *this; > } > + Metadata &operator=(Metadata &&other) > + { > + std::lock_guard<std::mutex> lock(mutex_); > + std::lock_guard<std::mutex> other_lock(other.mutex_); > + data_ = std::move(other.data_); > + other.data_.clear(); > + return *this; > + } https://en.cppreference.com/w/cpp/language/rule_of_three says you should also implement the copy and move constructors. I'm a bit concerned by the locks, but that's a separate question. > template<typename T> T *GetLocked(std::string const &tag) > { > // This allows in-place access to the Metadata contents,
Hi Laurent, Thank you for your feedback. On Fri, 16 Apr 2021 at 17:43, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Fri, Apr 16, 2021 at 11:31:40AM +0100, Naushir Patuck wrote: > > Add the move operator implementation for the RPiController::Metadata > > class. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/metadata.hpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/ipa/raspberrypi/controller/metadata.hpp > b/src/ipa/raspberrypi/controller/metadata.hpp > > index 4f44ffc6771c..c55194040f2f 100644 > > --- a/src/ipa/raspberrypi/controller/metadata.hpp > > +++ b/src/ipa/raspberrypi/controller/metadata.hpp > > @@ -45,6 +45,14 @@ public: > > data_ = other.data_; > > return *this; > > } > > + Metadata &operator=(Metadata &&other) > > + { > > + std::lock_guard<std::mutex> lock(mutex_); > > + std::lock_guard<std::mutex> other_lock(other.mutex_); > > + data_ = std::move(other.data_); > > + other.data_.clear(); > > + return *this; > > + } > > https://en.cppreference.com/w/cpp/language/rule_of_three says you should > also implement the copy and move constructors. > Indeed! I'll add the copy and move constructors in the next revision. > > I'm a bit concerned by the locks, but that's a separate question. > Fire away :-) Regards, Naush > > > template<typename T> T *GetLocked(std::string const &tag) > > { > > // This allows in-place access to the Metadata contents, > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp index 4f44ffc6771c..c55194040f2f 100644 --- a/src/ipa/raspberrypi/controller/metadata.hpp +++ b/src/ipa/raspberrypi/controller/metadata.hpp @@ -45,6 +45,14 @@ public: data_ = other.data_; return *this; } + Metadata &operator=(Metadata &&other) + { + std::lock_guard<std::mutex> lock(mutex_); + std::lock_guard<std::mutex> other_lock(other.mutex_); + data_ = std::move(other.data_); + other.data_.clear(); + return *this; + } template<typename T> T *GetLocked(std::string const &tag) { // This allows in-place access to the Metadata contents,
Add the move operator implementation for the RPiController::Metadata class. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/controller/metadata.hpp | 8 ++++++++ 1 file changed, 8 insertions(+)