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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
