[PATCH] D100137: [JumpThreading] merge debug info when merging select+br

Thu Apr 8 16:02:19 PDT 2021

MaskRay added a comment.

In D100137#2677851 <https://reviews.llvm.org/D100137#2677851>, @dblaikie wrote:

>> Link: b/184085493
>> Link: https://bugs.llvm.org/show_bug.cgi?id=39531
>> Link: https://www.llvm.org/docs/HowToUpdateDebugInfo.html#when-to-merge-instruction-locations
>> Signed-off-by: Nick Desaulniers <ndesaulniers at google.com>
>
> We don't have any tag styling like this in LLVM - maybe include some sentences about these links? (& if you like you can write the bug number with "PR39531")

I agree that Signed-off-by: should be dropped, and Link: should be replaced with inline references. And concise message is a good property.
(I am not quite used to the verbose Linux kernel style.)

> oh, and at least Google tries not to include links to internal bugs in upstream contributions (not an entirely consistent policy, Apple folks regularly link to their internal bug tracker & I'm sure there are some google bugs referenced upstream, though we try not to). And "Signed off by" seems inaccurate (& Phabricator will add "Reviewed By" I think anyway, which is usually the thing we might want to record)

Yeah, mostly a courtesy to other contributors, because links to internal things are completely not useful to others.
You can find such links more in other projects, e.g. chromium (https://chromium.googlesource.com/chromium/src/+/4fa3314af759654ed05c1d085210e92655cc7713)
but for llvm-project such links are rare. I don't object to including a link, though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100137