[Lldb-commits] [PATCH] D97165: [lldb] Add deref support and tests to shared_ptr synthetic

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

You can use `expect_var_path` to test `frame var` (LLDB calls the frame var input "expressions paths", hence the `_path` suffix). That way you can test all of these things for you without having to rely on just finding substrs in the output or having to depend on the way LLDB prints the result:

  self.expect_var_path("sp_str", summary="\"hello\" strong=1 weak=1",
                       type="std::shared_ptr<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >",
      children = [
          ValueCheck(name="__ptr_", summary="\"hello\"")
      ]
  )



================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py:11
+
+class LibcxSharedPtrDataFormatterTestCase(TestBase):
+
----------------
`Libcx` typo. FWIW, you don't have to give these classes a unique name anymore.


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py:17
+    def test_with_run_command(self):
+        """Test that that file and class static variables display correctly."""
+        self.build()
----------------
Comment + function name don't really fit to the test


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py:21
+        (self.target, self.process, _, bkpt) = lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
+
----------------
You only need `lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec("main.cpp"))` (the variable assignments and so on aren't necessary)


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/TestDataFormatterLibcxxSharedPtr.py:25
+            substrs=['(std::shared_ptr<int>) sp_empty = nullptr {',
+                               '__ptr_ = 0x0',
+                               '}'])
----------------
FWIW, this isn't really testing anything. This just tests that the pointer starts with '0x0', but even non-nullptrs start usually with 0x0 as we pad them with zeroes.

In theory you could test that this is equal to 0x0000000000000000 but then we have the test suite depend on the pointer size.

I think the best solution is to have the test suite figure out what a hex nullptr is on the target and then just provide that as utility variable in lldbtest. But that's out of the scope for this test, so I would say you can just leave it out until someone has time to implement that.


================
Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/shared_ptr/main.cpp:1
+#include <cstdio>
+#include <memory>
----------------
Unused include?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97165