[Lldb-commits] [PATCH] D83728: [lldb] Make `process connect` blocking in synchronous mode.

Mon Jul 13 16:16:32 PDT 2020

jingham added a comment.

Couple of minor comments.

================
Comment at: lldb/include/lldb/Target/Platform.h:855
 protected:
+  lldb::ProcessSP DoConnectProcess(llvm::StringRef connect_url,
+                                   llvm::StringRef plugin_name,
----------------
Even though this is a private method it's still worth documenting the fact that you are switching off of "stream != nullptr" to determine sync vrs. async attach.  That's not obvious.

================
Comment at: lldb/source/Target/Platform.cpp:1834
   if (error.Fail())
     return nullptr;

----------------
If you fail here you leave the process hijacked.  That doesn't matter because if "ConnectRemote" fails, you aren't going to have much to listen to anyway.  But it still looks odd.  I'm surprised we don't have some RAII-dingus for process hijacking, but anyway, it's good practice to undo this in the error branch.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83728