[PATCH] D135166: [DebugInfo] getMergedLocation: Maintain the line number if they match

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Hi, sorry for the delay following up on this.

I just wanted to mention that it turns out that SCE debugger tuning (`-gsce`) supresses column info by default and that our debugger ignores it. So no objections from me on that front.

I've left some inline nits, and I think it would be a good idea to update the function's comment in `llvm/IR/DebugInfoMetadata.h` too. Otherwise, I'm happy, thanks!



================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:119
+  unsigned Col = LocA->getColumn();
+  auto AdvanceToParentLoc = [&S, &L, &Line, &Col]() {
     S = S->getScope();
----------------
If I'm reading this right, `AdvanceToParentLoc` walks the entire scope chain from the source location up to file scope. Then walks the entire scope chain of the inlined-at location of the original source location up to file scope. Then, the entire scope chain of the inline-at location of that one, and so on. 

Is that right? I think it could be useful to add a comment explaining the traversal.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:134
   }
+
   S = LocB->getScope();
----------------
IMO it would be useful to add a comment here. Something along the lines of "Traverse the scope tree of `LocB` looking for the innermost match with `LocB`" maybe?


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:143
+      if (MatchLoc != Locations.end()) {
+        // if the lines match, keep the line, but set the column to '0'
+        bool SameLine = Line == MatchLoc->second.first;
----------------
nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135166