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

[Raphael Isemann via Phabricator via lldb-commits]

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
+  return LLDB_RECORD_RESULT(
+      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:
```lang=c++
// this is not exactly the usual meaning of stripping typedefs.
const bool stripped_typedef = true;
GetPossibleMatches(
....
  stripped_typedef);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72133