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 lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits