[PATCH] D103502: [DebugInfo] Bug 41152 - Improve dumping of empty location expressions

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM once my comments have been addressed.



================
Comment at: llvm/test/MC/X86/dwarf-improve-empty-location-exp.test:1
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj %s -o %t.o
+# RUN: llvm-dwarfdump %t.o --debug-loclists | FileCheck %s
----------------
As this test is about testing functionality in the DebugInfo library, I'd move it into llvm/test/DebugInfo/X86.

Also, I'd rename the test file to "dwarf-empty-expression.s":
  - In test names, don't use terms like "improve" or "fix" or similar, since the test name should be independent of the commit (i.e. the test will still be relevant long after the commit has been made).
  - Use "expression", to avoid abbreviations.
  - I don't think you need to be explicit about "location" as the code under test is about the expression printing (expressions may be used for locations, but that's not relevant to the code under test).
  - Use ".s" for the extension because ".s" is the file extension used for raw assembly files.


================
Comment at: mypatch.patch:1
+diff --git a/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp b/llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp
+index d5015f00f1cf..4b9be85f6885 100644
----------------
I don't think you meant to add this file to your patch.


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

https://reviews.llvm.org/D103502