[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Raphael Isemann via Phabricator via lldb-commits]

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