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

[Amy Huang via Phabricator via llvm-commits]

akhuang marked an inline comment as done.
akhuang added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:470
+      if (Lines.hasColumnInfo() && ColIt == ColsEnd)
+        continue;
+
----------------
amccarth wrote:
> 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.
Makes sense. I was avoiding using asserts in case some section happened to be messed up, in which case we would just skip it instead of asserting. (Although I don't know if this is actually something that could happen.)

I think my `if` statement is just the inverse of your suggested assert? It continues if it claims there is column info and the column info is empty.

The original test case in `llvm/test/tools/llvm-symbolizer/pdb/pdb-native.test` doesn't have column info and passes. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81950