JDevlieghere added inline comments.
================
Comment at: lldb/examples/python/scripted_process/scripted_process.py:242
+ """
+ pass
+
----------------
Should `eStateStopped` be the default?
================
Comment at: lldb/examples/python/scripted_process/scripted_process.py:244
+
+ # @abstractmethod
+ def get_queue(self):
----------------
Why's this commented out?
================
Comment at: lldb/include/lldb/Interpreter/ScriptedProcessInterface.h:77
+ StructuredData::GenericSP
+ CreatePluginObject(const llvm::StringRef class_name,
+ ExecutionContext &exe_ctx,
----------------
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:105
+const char *ScriptedThread::GetName() {
+ if (m_name.empty()) {
+ CheckInterpreterAndScriptObject();
----------------
We're caching the name and the queue, is this an optimization (to avoid calling
into Python over and over) or are there assumptions that will break if these
are not stable? If so, should that be something we call out on the Python side
or alternatively, should we allow this to change and not cache it?
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:141-144
+ uint32_t concrete_frame_idx = 0;
+
+ if (frame)
+ concrete_frame_idx = frame->GetConcreteFrameIndex();
----------------
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:152-156
+ auto error_with_message = [](llvm::StringRef message) {
+ LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+ "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
+ return false;
+ };
----------------
Seems like you have this lambda in a lot of places, I wonder if it's worth the
trade-off of making this a static function that takes `__FUNCTION__` as an
extra argument.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:158-160
+ if (m_cached_stop_info_sp) {
+ SetStopInfo(m_cached_stop_info_sp);
+ } else {
----------------
You call `SetStopInfo(m_cached_stop_info_sp);` on line 211, so we could drop
the case for `m_cached_stop_info_sp` being true?
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp:128-133
+ if (!obj || !obj->IsValid() || error.Fail()) {
+ return error_with_message(llvm::Twine("Null or invalid object (" +
+ llvm::Twine(error.AsCString()) +
+ llvm::Twine(")."))
+ .str());
+ }
----------------
This code is duplicated over and over for objects and dicts. Seems like a
templated function could help here. Then you wouldn't have to worry about being
more verbose and differentiating the different errors (null vs invalid vs
error). Also, is there always an error when `!obj` or `!obj->IsValid()` or are
there cases where this might print something like "Null or invalid object ()"?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107585/new/
https://reviews.llvm.org/D107585
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
