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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Sorry about my review latency.

This mostly looks really good.  I'm not trying to pick on you with the efficiency things.  I've just been watching too many YouTube videos of Chandler's efficiency talks.  Please consider the inline comments as alternatives to consider rather than change requests.



================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:25
+    std::vector<NativeLineNumber> LineNums)
+    : Lines(LineNums), Index(0) {}
+
----------------
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.)


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:31
+
+std::unique_ptr<IPDBLineNumber>
+NativeEnumLineNumbers::getChildAtIndex(uint32_t N) const {
----------------
I'm trying to weigh the pros and cons of returning by `std::unique_ptr<T>` versus `ErrorOr<T>` or `Expected<T>`.  I don't think either of the latter ones requires a memory allocation.  But maybe there's API precedent here?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:35
+    return nullptr;
+  return std::make_unique<NativeLineNumber>(Lines[N]);
+}
----------------
I'm wondering whether this should set the Index member to N (or N+1) before returning.  If a caller calls getChildAtIndex(10) and then calls getNext(), would they expect child 0 or child 10 or child 11?

Is there an similar API that sets a precedent for the expectation here?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:529
+  Optional<uint16_t> EndModi = getModuleIndexForAddr(VA + Length);
+  std::vector<NativeLineNumber> LineNumbers;
+  while (Modi <= *EndModi) {
----------------
Will `LineNumbers` generally contain a small number of entries?  If it's usually or almost always small, it could be worth making it a SmallVector to eliminate extra allocations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962