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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth accepted this revision.
amccarth marked 2 inline comments as done.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumLineNumbers.cpp:31
+
+std::unique_ptr<IPDBLineNumber>
+NativeEnumLineNumbers::getChildAtIndex(uint32_t N) const {
----------------
akhuang wrote:
> 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. 
Yeah, that makes sense.  OK, question withdrawn.  `std::unique_ptr` is fine as is.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:529
+  Optional<uint16_t> EndModi = getModuleIndexForAddr(VA + Length);
+  std::vector<NativeLineNumber> LineNumbers;
+  while (Modi <= *EndModi) {
----------------
akhuang wrote:
> 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.
OK, that's more than I expected.  `std::vector` seems an appropriate choice.  Thanks for checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80962