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

[Amy Huang via Phabricator via llvm-commits]

akhuang added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:31
+
+std::unique_ptr<IPDBLineNumber>
+NativeEnumLineNumbers::getChildAtIndex(uint32_t N) const {
----------------
amccarth wrote:
> 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?
Yeah, I think they match the DIA APIs?  This class is inheriting from the IPDBEnumChildren interface. 


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:35
+    return nullptr;
+  return std::make_unique<NativeLineNumber>(Lines[N]);
+}
----------------
amccarth wrote:
> 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?
The other NativeEnum-- classes act the same way, such as `NativeEnumModules::getChildAtIndex`.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:529
+  Optional<uint16_t> EndModi = getModuleIndexForAddr(VA + Length);
+  std::vector<NativeLineNumber> LineNumbers;
+  while (Modi <= *EndModi) {
----------------
amccarth wrote:
> 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.
Hm, I checked how many entries a `NativeEnumLineNumbers` normally has (using a clang crash for testing) and it appears it's often somewhere between 20 and 200.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962