[Lldb-commits] [PATCH] D125325: Pass plugin_name in SBProcess::SaveCore

[Greg Clayton via Phabricator via lldb-commits]

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

Looking great, just a few more fixes. Thanks for making the changes.



================
Comment at: lldb/include/lldb/API/SBProcess.h:348
+  /// \param[in] core_style - Specify the style of a core file to save.
+  /// "minidump" plug-in supports only "eSaveCoreStackOnly".
+  lldb::SBError SaveCore(const char *file_name, const char *flavor,
----------------
Might not need to mention this in the header file as long as the SBError mentions that it isn't supported in the error message.


================
Comment at: lldb/source/API/SBProcess.cpp:1147
+                                  SaveCoreStyle core_style) {
+  LLDB_INSTRUMENT_VA(this, file_name, flavor);
 
----------------
missing core_style in the LLDB_INSTRUMENT_VA macro


================
Comment at: lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py:47
+            # validate savinig via SBProcess
+            lldb.SBProcess().SaveCore(core_sb, "minidump", "stack")
             self.assertTrue(os.path.isfile(core))
----------------
The 3rd argument is being passed by string value which isn't correct. We need to use the enumeration.


================
Comment at: lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py:47
+            # validate savinig via SBProcess
+            lldb.SBProcess().SaveCore(core_sb, "minidump", "stack")
             self.assertTrue(os.path.isfile(core))
----------------
clayborg wrote:
> The 3rd argument is being passed by string value which isn't correct. We need to use the enumeration.
We might think about adding a test for "minidump" to save it with a style of "lldb.eSaveCoreFull" and "lldb.eSaveCoreDirtyOnly" and verify we get an error back. If this ever changes, then we can modify the test.


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

https://reviews.llvm.org/D125325