[PATCH] D80962: [NativeSession] Implement findLineNumbersByAddress functions for native llvm-symbolizer support.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:25
+    std::vector<NativeLineNumber> LineNums)
+    : Lines(LineNums), Index(0) {}
+
----------------
amccarth wrote:
> I know I argued the other way in a different patch, but the details were different.  Here I think you should take the parameter by rvalue reference.  I could be wrong here, but here's how I'm thinking about this.
> 
> As far as I can tell, the only invocation of this constructor is using `std::move`, which I don't think helps when the argument is accepted by value.  Then the argument is copied into the Lines member.  So that's two copies total.  (Right?)
> 
> If you changed the initializer to be `Lines(std::move(LineNums))`, then the argument could be moved into Lines.  In total, you'd up with a copy and a move, which is better.
> 
> If you changed the constructor to accept by rvalue reference, then the `std::move` at the call site becomes useful.  Combine with with a `std::move` into Lines, and I think that would eliminate all copies and simply be two moves.  (Moving a `std::vector` is a win over copying it.)
Some of what I said is wrong.  The existing `std::move` does seem to avoid a copy based on my experiments with Compiler Explorer ("godbolt").  Despite my error, I still think my recommendations are still good ideas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962