[PATCH] D81950: [NativeSession] Add column numbers to NativeLineNumber.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

This looks right, but I'd like to see a test.

llvm-pdbutil is used in a DIA test for line numbers (llvm\test\DebugInfo\PDB\DIA\pdbdump-linenumbers.test), so perhaps that could be the model for a native one that also checks the column numbers.  That would require a tweak to llvm-pdbutil to make essentially the same change as you have here, to include column numbers in the output when they're present.



================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:467
+      auto ColsEnd = Group.Columns.end();
+      for (; LineIt != LinesEnd; ++LineIt) {
+        uint64_t VA = Session.getVAFromSectOffset(RelocSegment,
----------------
As I understand it, the line numbers and column numbers are in parallel arrays (streams).  So, for each line number you take the next column number (if there are column numbers at all).

Assuming that's right:

I don't see a need to change the for loop to use the line number iterators.  Yeah, it creates some source code symmetry with the column number iterators, but I'm not sure that makes that more understandable here.  I won't block on it, but I'd prefer the original `for (const LineNumberEntry &LN : Group.LineNumbers)`.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:472
+        uint32_t ColNum = 0;
+        if (Lines.hasColumnInfo() && ColIt != ColsEnd) {
+          ColNum = ColIt->StartColumn;
----------------
It seems that if there is no column info, then `ColIt` would always equal `ColsEnd`, so perhaps that first part of the condition is not necessary.  Perhaps, just before the loop, you could assert that either `Lines.hasColumnInfo()` or that `ColIt` is `ColsEnd` and then simplify the condition here.

The assertion would be a nice place to add a comment like

```
// If there are column numbers, then they are in a parallel stream to the line numbers.
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81950