dblaikie added a comment.

Probably worth separating these changes/fixes/tests (looks like 3 different 
changes?) - at least it'd help me understand what each one of the changes does, 
separately (I'm especially curious about the EmitScalarConversion one and would 
like to better understand why the generic Expr handling for ApplyDebugLocation 
isn't working here, if it can be made more robust and/or if the proposed 
solution could be generalized a bit/moved further up in the stack). And is 
there an existing test file/cases these could be put near?



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4293
+  {
+    // There is no need to emit line number for unconditional branch.
+    auto NL = ApplyDebugLocation::CreateEmpty(CGF);
----------------
rnk wrote:
> I see this is consistent with LAnd handling above, but is there a principled 
> reason for using an empty location here? If I was stopped on this 
> unconditional branch instruction in the debugger, I would expect to see the 
> location of the logical operator.
> 
> No need to change behavior in this patch, I'm just wondering if we can remove 
> this as a followup.
> 
> I dug into the blame, and the comment about "no need" for a location comes 
> from 2011:
> http://github.com/llvm/llvm-project/commit/4d7612744f080d2c0a8639c1f5e41b691e1bba55
> The reasoning seems to be that it improves the gdb test suite.
I /think/ I remember encountering these issues independently/as well when I was 
working on the gdb test suite maybe a couple of years later. Hmm, guess it 
might've overlapped with this work.

6f2e41e0d4421ab376801bfb88d3f197a8e96994 / r128471 provides maybe a bit more 
context: "Do not line number entry for unconditional branches. Usually, users 
do not want to stop at closing '}'."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92606

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to