[Lldb-commits] [PATCH] D72133: Data formatters: Look through array element typedefs

Raphael Isemann via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Jan 3 04:10:07 PST 2020

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

I have some comments but otherwise this patch looks good to me. Thanks!

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/TestArrayTypedef.py:2
+Test lldb data formatter subsystem.
I rather have no comment than a generic one (it's kinda obvious what is tested here from the name).

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/TestArrayTypedef.py:13
+    mydir = TestBase.compute_mydir(__file__)
I think we can mark this as `NO_DEBUG_INFO_TESTCASE = True` to only run this once.

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/array_typedef/main.cpp:1
+//===-- main.cpp ------------------------------------------------*- C++ -*-===//
We usually don't add license headers to test files.

Comment at: lldb/source/API/SBType.cpp:218
+      SBType(TypeImplSP(new TypeImpl(canonical_type.GetArrayElementType()))));
I get that this is to preserve the previous SB API behavior but I think it's better if we keep this method a simple wrapper without extra functionality. That we return the canonical type seems like a bug for me, so it's IMHO fine to change this behavior.

Comment at: lldb/source/DataFormatters/FormatManager.cpp:241
+  uint64_t array_size;
+  if (compiler_type.IsArrayType(nullptr, &array_size, nullptr)) {
Maybe add a comment that we strip here typedefs of array element types.

Comment at: lldb/source/DataFormatters/FormatManager.cpp:245
+    if (element_type.IsTypedefType()) {
+      CompilerType deffed_array_type =
+          element_type.GetTypedefedType().GetArrayType(array_size);
Maybe add a comment that this is the original array type with the element type typedef desugared.

Comment at: lldb/source/DataFormatters/FormatManager.cpp:251
+          use_dynamic, entries, did_strip_ptr, did_strip_ref,
+          true); // this is not exactly the usual meaning of stripping typedefs
+    }
I know this is copied from a above but I think if this is more readable:
// this is not exactly the usual meaning of stripping typedefs.
const bool stripped_typedef = true;

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list