[Lldb-commits] [PATCH] D52851: Adding support to step into the callable wrapped by libc++ std::function

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

Some basic comments. Haven't looked at the implementation very closely, I'll do that probably tomorrow. Thanks for working on this!



================
Comment at: include/lldb/Target/CPPLanguageRuntime.h:59-60
 
+  lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
+                                                  bool stop_others);
+
----------------
Please add a doxygen comment to this function. (I understand the surrounding ones are not commented, but we should start somewhere)/


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:1-3
+"""
+Test lldb data formatter subsystem.
+"""
----------------
This comment seems a little outdated.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:8-9
+
+import os
+import time
+import lldb
----------------
I'm not sure you need these.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:27-28
+
+    @add_test_categories(["libc++"])
+    def test(self):
+        """Test that std::function as defined by libc++ is correctly printed by LLDB"""
----------------
Is this affected by the debug info variant used? (i.e. dSYM/gmodules/DWARF/dwo). 
If not, this could be a `NO_DEBUG_INFO` test


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:56-73
+        self.assertEqual( self.thread.GetFrameAtIndex(0).GetLineEntry().GetLine(), 13 ) ;
+        self.process.Continue()
+
+        self.thread.StepInto()
+        self.assertEqual( self.thread.GetFrameAtIndex(0).GetLineEntry().GetLine(), 28 ) ;
+        self.process.Continue()
+
----------------
All these lines check hardcoded make me a little nervous, as they're really fragile. Can you make them parametric at least?
so that if somebody slides down the test case they're still valid (i.e. base + offset)


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/main.cpp:1-9
+//===-- main.cpp --------------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
----------------
You don't need a license for the test case, I think.


================
Comment at: source/Target/ThreadPlanStepThrough.cpp:99-105
+
+    CPPLanguageRuntime *cpp_runtime =
+        m_thread.GetProcess()->GetCPPLanguageRuntime();
+
+    if (cpp_runtime)
+      m_sub_plan_sp =
+          cpp_runtime->GetStepThroughTrampolinePlan(m_thread, m_stop_others);
----------------
Can you add a comment here?


https://reviews.llvm.org/D52851