[PATCH] D103502: Bug 41152 - [DebugInfo] Better dumping of empty location expression

Mon Jun 7 11:35:51 PDT 2021

dblaikie accepted this revision.
dblaikie added a comment.

In D103502#2801992 <https://reviews.llvm.org/D103502#2801992>, @jhenderson wrote:

> Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.

Sure, I'm OK with it. Hadn't given it a great deal of thought/look other than out of concern for how the DWARF parsing libraries might be impacted.

> If I'd written this patch myself, I'd have probably also added a new dedicated test that explicitly checks the output for empty location ranges. It looks like the existing test is producing an empty range as a side-effect of the behaviour under test, and so it would be quite possible that the test coverage would be lost if this beahviour changed/the testing method changed for it. What do others think?

Yep, sounds like a fair point to me.

>> Bug 41152 - [DebugInfo] Better dumping of empty location expression
>
> As a tip for this summary - we usually put tags at the start of the summary, so I'd relabel this as "[DebugInfo] Bug 41152 - Improve dumping of empty location expressions". Note also the slight change of wording too ("improve" is a verb, making the summary a full imperative sentence).

Also can be nice to write the bug as "PR41152" rather than "Bug 41152" - it's a bit more obscure, but PR* syntax is pretty common in LLVM and a few characters shorter to fit in the commit message limits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103502