[Lldb-commits] [PATCH] D96176: Implement jAttachWait

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inlined comment about the packet name issue and let me know what you think



================
Comment at: lldb/include/lldb/Target/Process.h:182-183
     m_wait_for_launch = false;
+    m_wait_for_launch_interval = llvm::Optional<uint64_t>();
+    m_wait_for_launch_duration = llvm::Optional<uint64_t>();
     m_ignore_existing = true;
----------------



================
Comment at: lldb/include/lldb/Target/Process.h:223-224
   bool m_wait_for_launch;
+  llvm::Optional<uint64_t> m_wait_for_launch_interval;
+  llvm::Optional<uint64_t> m_wait_for_launch_duration;
   bool m_ignore_existing;
----------------
Should we attach _usec and sec to these names to make it clear what the units are?


================
Comment at: lldb/include/lldb/Utility/StringExtractorGDBRemote.h:136
     eServerPacketType_vAttachOrWait,
+    eServerPacketType_jAttachWait,
     eServerPacketType_vAttachName,
----------------
So we currently have 4 flavors from the old way: vAttach, vAttachWait vAttachOrWait, vAttachName. Do we want to possibly make a "jAttach" that works for all cases? The new "jAttach" could be powerful enough to do it all in one packet if needed. I just worry about attaching the "Wait" to the "jAttachWait" command name in case we want to use it for all of the above cases at a later point. 

A "jAttach" could implement all of:
- attach to existing pid
- attach to by name
  - unique existing process
  - wait
    - allow existing (true/false)
    - poll interval (time in usec)
    - wait duration (time in sec)

Not that we need to add support for all of these flavor with this patch, I'm just trying to forward think about the future in case other attach flags are added to any flavor of attach. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96176