[Lldb-commits] [PATCH] D125509: [LLDB][NFC] Decouple dwarf location table from DWARFExpression.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Separating the notion of an expression and an expression list seems worthwhile. I did a very high level pass but I'll take a more detailed look tomorrow.



================
Comment at: lldb/include/lldb/Expression/DWARFExpressionList.h:1-2
+//===-- DWARFExpressionList.h ---------------------------------------*- C++
+//-*-===//
+//
----------------
Formatting


================
Comment at: lldb/include/lldb/Expression/DWARFExpressionList.h:22
+
+// It's the interface to get or evaluate a DWARFExpression.
+class DWARFExpressionList {
----------------
This should be a Doxygen comment. But more importantly, this doesn't really tell me what the class does. DWARFExpressionList implies it holds a list of expressions so that seems useful to include there.


================
Comment at: lldb/include/lldb/Expression/DWARFExpressionList.h:24
+class DWARFExpressionList {
+private:
+  // RangeDataVector requires a comparator for DWARFExpression, but it doesn't
----------------
I think we generally put public first, followed by protected and private last. 


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/dwp.s:22-23
 # SYMBOLS-NEXT:   Block{{.*}}, ranges = [0x00000000-0x00000001)
-# SYMBOLS-NEXT:     Variable{{.*}}, name = "x", {{.*}}, location = 
-# SYMBOLS-NEXT:       DW_LLE_startx_length   (0x0000000000000001, 0x0000000000000001): DW_OP_reg0 RAX
+# SYMBOLS-NEXT:     Variable{{.*}}, name = "x", {{.*}}, location =
+# SYMBOLS-NEXT:       [0x0000000000000000, 0x0000000000000001): DW_OP_reg0 RAX
 # SYMBOLS-EMPTY:
----------------
I guess the patch is not NFC is the output changes? Would it be possible to split the functional and non-functional part of this patch into separate patches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125509