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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 4 inline comments as done.
mib added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:66
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  return static_cast<bool>(GetGenericInteger("shuold_stop"));
 }
----------------
teemperor wrote:
> Unrelated to the change here, but that looks like a typo in the string.
Thanks for catching that!


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:294
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  return static_cast<bool>(GetGenericInteger("is_alive"));
 }
----------------
teemperor wrote:
> 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.
This is just the interface between Python and C++, the object sanity check is done above in `ScriptedProcess::IsAlive`.


================
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);
----------------
teemperor wrote:
> 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.
There is actually a `PythonInteger` class already but in order to extract the scalar, I have to make a proxy `StructuredData::Integer` to get the `uint64_t` value. Notice, in this case, the return type is not `unsigned long long` anymore ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788