[Lldb-commits] [PATCH] D107521: [lldb/Plugins] Introduce Scripted Interface Factory

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This is a good start, but it seems like there's still a lot of boilerplate once you get to the python side that could be trimmed down.

For instance, ScriptedProcessPythonInterface::GetThreadWithID is almost entirely generic.  The only things that make it specific to this use are the method name that's called, and the fact that it passes an lldb::tid_t to and returns an SBStructuredData.

You've started to take care of the latter by building up a set of generic "call a method, return X" routines, and surely an SBStructuredData one will be pretty common.

Then you can add a way to pass arguments to the generic routines.  Maybe an array of some little struct that has either the duple "python format string/value" or a PythonObject so the dispatcher can call the code appropriately?



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp:32-44
+llvm::Optional<unsigned long long>
+ScriptedPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
+  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+                 Locker::FreeLock);
+
+  if (!m_object_instance_sp)
+    return llvm::None;
----------------
It seems like there is a whole lot of duplicated code here.  The only difference is in what we do with the returned PythonObject.

Maybe we should have a centralized "dispatch" method and then these routines differ in the conversion.

Also, it seems a shame to throw away error info, but that's what the GetGenericInteger & GetGenericString do when the object instance isn't valid, etc.  llvm::Optional is good for forcing validity checks, but has no information on why the operation didn't succeed.  It would be better to have all these methods take a status object for errors.  You can still return the result wrapped in an optional.

When you are developing one of these classes, it would be nice to be told "no such method" for instance so you can go check if you misspelled it, or something, rather than just getting llvm::None that eventually results in some more remote error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107521