[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting when reading memory in Scripted Process

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I kinda like it. One thing that I think would help with the readability is if the "transformation" methods were grouped according to the type of the object being transformed, rather than according to the direction. So something like:

  // general transform
  // general reverse
  // Status transform
  // Status reverse

instead of

  // general transform
  // Status transform
  // general reverse
  // Status reverse

Also I don't think that these structs (`transformation`, `reverse_transformation`) wrapping the transformation functions are really necessary. It's true that one cannot partially specialize functions, but one of the reasons for that is this is normally not necessary -- regular function overloading <https://godbolt.org/z/114KeeK3f> can do most of that as well.



================
Comment at: lldb/bindings/python/python-swigsafecast.swig:36
 
+PythonObject ToSWIGWrapper(Status& status) {
+  return ToSWIGHelper(new lldb::SBError(status), SWIGTYPE_p_lldb__SBError);
----------------
add `const` ?


================
Comment at: lldb/source/API/SBError.cpp:29
+SBError::SBError(const lldb_private::Status &status)
+    : m_opaque_up(new Status(status.ToError())) {
+  LLDB_INSTRUMENT_VA(this, status);
----------------
That's cute, but you can just call the copy constructor (`new Status(status)`)


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:124
+  Status py_error;
+  // TODO: Log py_error after call if failed
+  return Dispatch<lldb::DataExtractorSP>("read_memory_at_address", py_error,
----------------
or assign it to the `error` argument instead?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:11-20
 
-#include "lldb/Host/Config.h"
-
-#if LLDB_ENABLE_PYTHON
+#include <sstream>
+#include <tuple>
+#include <type_traits>
+#include <utility>
 
+#include "lldb/Host/Config.h"
----------------
move this inside `#if LLDB_ENABLE_PYTHON`


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:91-119
     if (sizeof...(Args) > 0) {
-      FormatArgs(format_buffer, args...);
+      std::apply(
+          [&format_buffer, this](auto... args) {
+            this->FormatArgs(format_buffer, args...);
+          },
+          transformed_args);
       // TODO: make `const char *` when removing support for Python 2.
----------------
Can this be a call to `PythonObject::CallMethod` (via std::apply and everything). If not, why not?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:150
+    // Call SWIG Wrapper function
+    python::PythonObject py_obj = python::ToSWIGWrapper(arg);
+    return py_obj.release();
----------------
PythonObject::CallMethod knows how to unwrap a PythonObject argument, so ideally, we'd return py_obj directly here.


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

https://reviews.llvm.org/D134033