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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
