[Lldb-commits] [PATCH] D69080: eliminate one form of PythonObject::Reset()

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna marked 3 inline comments as done.
lawrence_danna added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:235-245
   PythonObject &operator=(const PythonObject &other) {
     Reset(PyRefType::Borrowed, other.get());
     return *this;
   }
 
-  void Reset(PythonObject &&other) {
+  PythonObject &operator=(PythonObject &&other) {
     Reset();
----------------
labath wrote:
> lawrence_danna wrote:
> > labath wrote:
> > > You can consider simplifying this further down to a "universal"/"sink" `operator=(PythonObject other)`. Since the object is really just a pointer, the extra object being created won't hurt (in fact, the removal of `&`-indirection might make things faster).
> > wouldn't that result in an extra retain and release every time a PythonObject was copied instead of referenced or moved?
> No, it shouldn't, because the temporary PythonObject will be move-constructed (== no refcount traffic), if the operator= is called with an xvalue (if the rhs was not an xvalue, then you wouldn't end up calling the `&&` overload anyway). Then you can move the temporary object into *this, and avoid refcount traffic again.
> 
> So, there is an additional PythonObject created, but it's move-constructed if possible, which should be efficient, if I understand these classes correctly. This is the recommended practice (at least by some) when you don't want to squeeze every last nanosecond of performance..
How do you move the temporary object into *this, if you only have `operator=(PythonObject other)` to assign with?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69080/new/

https://reviews.llvm.org/D69080