aprantl added a comment. Thanks, this looks really good! I have a couple of small comments inline.
================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.h:77 +// libc++ std::span<> +bool LibcxxSpanSummaryProvider(ValueObject &valobj, Stream &stream, ---------------- `/// Formatter for libc++ std::span<>.` ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:40 +private: + ValueObject *m_start = nullptr; ///< First element of span + CompilerType m_element_type{}; ///< Type of span elements ---------------- `///< First element of span.` ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:55 + ~LibcxxStdSpanSyntheticFrontEnd() { + // this needs to stay around because it's a child object who will follow + // its parent's life cycle ---------------- // This ... ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:56 + // this needs to stay around because it's a child object who will follow + // its parent's life cycle + // delete m_start; ---------------- . ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:57 + // its parent's life cycle + // delete m_start; +} ---------------- I would just delete this last line IMHO, it's more confusing than helping. Or maybe just say that m_start is owned by s shared_ptr elsewhere. ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:80 + +/* + * std::span can either be instantiated with a compile-time known ---------------- I would move this into a doxygen comment on the function declaration. ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:82 + * std::span can either be instantiated with a compile-time known + * extent or a std::dynaic_extent (this is the default if only the + * type template argument is provided). The layout of std::span ---------------- dymanic ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:103 +bool lldb_private::formatters::LibcxxStdSpanSyntheticFrontEnd::Update() { + // Get element type + ValueObjectSP data_type_finder_sp( ---------------- // Get element type. ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:111 + + // Get element size + if (llvm::Optional<uint64_t> size = m_element_type.GetByteSize(nullptr)) { ---------------- dito ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp:116 + // Get data + if (m_element_size > 0) { + m_start = data_type_finder_sp.get(); ---------------- Technically LLVM doesn't use {} on single-statement bodies. ================ Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/Makefile:5 + +CXXFLAGS_EXTRAS := -std=c++20 -O0 +include Makefile.rules ---------------- Michael137 wrote: > Might need to have some compiler version checks here since by default we > assume c++11 The -O0 should not be necessary. ================ Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/TestDataFormatterLibcxxSpan.py:53 + @add_test_categories(["libc++"]) + def test_with_run_command(self): + """Test that std::span variables are formatted correctly when printed.""" ---------------- We probably need to skip this on earlier compilers that don't support C++20. ================ Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/TestDataFormatterLibcxxSpan.py:71 + # Execute the cleanup function during test case tear down. + self.addTearDownHook(cleanup) + ---------------- I think the cleanups are no longer necessary because we run every test in isolation now. ================ Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp:10 + // Stop here to check by ref + return; +} ---------------- a more robust way to ensure code exists on this line would be `printf(" Stop here to check by ref")` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127481/new/ https://reviews.llvm.org/D127481 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits