[PATCH] D79269: [NativeSession] Implement NativeSession::findSymbolByAddress.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeFunctionSymbol.cpp:21
+                                           SymIndexId Id, codeview::ProcSym Sym)
+    : NativeRawSymbol(Session, PDB_SymType::Data, Id), Sym(std::move(Sym)) {}
+
----------------
labath wrote:
> amccarth wrote:
> > The reason for the `std::move` is not obvious (to me).  The constructor takes the Sym parameter by value (so it's a copy) and then stashes it in member Sym.  Is the goal to avoid a second copy from the argument to the member?
> > 
> > If it's expensive to copy, should the constructor take a `const &` and then copy the value (once) into the data member?
> > 
> > (And if you must use `std::move`, I think this file should include `<utility>`.)
> This is actually the recommended way to pass objects if your function "consumes" them and you don't care about every last drop of performance. The assertion that this will always incur a copy is not true. In fact, `const T &` guarantees a copy. This puts the choice into the hands of the caller. If the caller calls the constructor with an lvalue, there will be 1 copy (+ 1 move). If the caller calls this with an xvalue, there will be no copies (only two moves).
Two moves would require that `Sym` has a move constructor, right?  It looks like `Sym` is a `codeview::ProcSym`.  I suppose it might get a compiler-generated move constructor, but that class appears to be a collection of scalar types (enums, integers, and a non-owning raw pointer).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79269