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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

I think this is the last round of review so I'll just accept this modulo a few nits.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:190
+
+  StructuredData::ObjectSP structured_object(
+      py_return.CreateStructuredObject());
----------------
As discussed offline, I think with the API we got here I would much rather have the unhandled llvm::Expected from the old code than the strange StructuredData code that just silently hides errors. Let's revert this back to the old but with llvm::None and put a big FIXME there.

```
lang=c++
    llvm::Expected<unsigned long long> size = py_return.AsUnsignedLongLong();
    if (!size) {
      // FIXME: Handle error.
      return llvm::None;
    }
    return *size;
```


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:299
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits<lldb::pid_t>::max())
-             ? LLDB_INVALID_PROCESS_ID
-             : pid;
+  auto pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
----------------
Could you spell the `auto` out here? `llvm::Optional<uint64_t>` is I think simple enough.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:304
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  auto is_alive = GetGenericInteger("is_alive");
+
----------------
Same as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788