labath added a comment.

A couple of more comments from me. The two on the test are minor, but I think 
it would be good to resolve the one about the SB method placement soon, before 
this thing starts having users.



================
Comment at: lldb/bindings/interface/SBTarget.i:952-957
+    %feature("autodoc", "
+    Returns the platform's process extended crash information.") 
GetExtendedCrashInformation;
+    lldb::SBStructuredData
+    GetExtendedCrashInformation ();
+
+
----------------
I know it was my idea to have this on the SBTarget class, but I think we should 
move this once more :/. I mean, this is definitely better than the SBPlatform 
class (which is what I was really trying to say before), but I think SBProcess 
would be even better here. The SBTarget methods are things that (more or less) 
make sense even when you don't have an actual process around. However, "crash 
info" (I think) only makes sense once you have an actual process, and so it 
would make sense to have it somewhere near SBProcess::GetState/GetDescription. 
It would also be more symmetric with the CLI interface for this, which lives 
under the "process" command tree.

The internal interfaces might also be better off passing a Process object 
(instead of Target) around, but that is not as important..

Sorry. :(


================
Comment at: 
lldb/test/API/functionalities/process_crash_info/TestProcessCrashInfo.py:20
+        self.source = "main.c"
+        self.line = 3
+
----------------
We have a `line_number` utility function to avoid hardcoding line numbers like 
this.


================
Comment at: 
lldb/test/API/functionalities/process_crash_info/TestProcessCrashInfo.py:65-82
+    @skipUnlessDarwin
+    def test_before_launch(self):
+        """Test that lldb doesn't fetch the extended crash information
+        dictionnary from if the process wasn't launched yet."""
+        self.build()
+        target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+        self.assertTrue(target, VALID_TARGET)
----------------
I'm hoping that these two tests can actually run (and pass) on non-darwin 
systems too...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74657



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to