[Lldb-commits] [PATCH] D127481: [LLDB][formatters] Add formatter for libc++'s std::span

[Adrian Prantl via Phabricator via lldb-commits]

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