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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Very close now.  It looks like the change I requested led to a bug.  Once that's corrected, this'll look great.

Thanks for adding the test.  Perhaps you could generate PDBs both with and without column info and then the test could check that it works either way (which should have caught the new bug).  If there's already an existing test that catches the bug (skipping the line numbers whether there are no column numbers), that's sufficient.

Feel free to ping me if I've not been clear.



================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:470
+      if (Lines.hasColumnInfo() && ColIt == ColsEnd)
+        continue;
+
----------------
Sorry, I wasn't clear enough before because it looks like I confused you.  This `if` statement is unnecessary and, in fact, wrong.  You still want to go through the line numbers in this group even if there's no column information.

When I said assert, I literally meant an `assert` like this:

```
llvm_assert(Lines.hasColumnInfo() || ColIt == ColsEnd);
```

Note that it's `||`, not `&&`.  If we have column info, great.  If we don't, then ensure that the iterator alone makes that clear in order to use the simpler test in the loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81950