teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
Some nits but otherwise this seems like the right direction. Thanks!
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:66
bool ScriptedProcessPythonInterface::ShouldStop() {
- return GetGenericInteger("shuold_stop");
+ return static_cast<bool>(GetGenericInteger("shuold_stop"));
}
----------------
Unrelated to the change here, but that looks like a typo in the string.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:137
-size_t
+unsigned long long
ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name)
{
----------------
Could we return an `llvm::Optional<...>` here? Then we don't need the
`invalid_address` variable but instead just `return llvm::None` in the early
exits.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:294
bool ScriptedProcessPythonInterface::IsAlive() {
- return GetGenericInteger("is_alive");
+ return static_cast<bool>(GetGenericInteger("is_alive"));
}
----------------
I think this should return `false` when the underlying object is invalid? Bit
out of scope for this patch, so feel free to fix this in another review.
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h:54
protected:
- size_t GetGenericInteger(llvm::StringRef method_name);
+ unsigned long long GetGenericInteger(llvm::StringRef method_name);
Status GetStatusFromMethod(llvm::StringRef method_name);
----------------
Could you actually make a typedef for this? `typedef unsigned long long
PythonInteger` or so. cpython really like to change their API between versions
and this would make the potential fixups a bit easier if they decide to change
the return value.
We might actually want to put this typedef into the `PythonObject` API int the
future, but that's probably out-of-scope for this patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105788/new/
https://reviews.llvm.org/D105788
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
