[Lldb-commits] [PATCH] D98482: [lldb] Support for multiprocess extension [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Some initial remarks:

- the parsing function is clearly gdb-related, so it should go into StringExtractorGDBRemote.h, and not its base class
- the universalness of the function makes its interface pretty complicated, and bloats the call sites. Its probably good for the lowest layer, but maybe we could make things a lot cleaner if we created some more specialized functions on top of that. For example, most of the code (packets) statically knows whether it can accept `-1` or `0` for process or thread ids, so we would benefit from those checks being done in a central place. in particular, until we really start supporting multiple processes, all of the packets will be doing something like `if (pid != m_process->pid()) return error()`, so we could have a version that accepts a value that *must* be present as the pid field. That would also allow us to mostly abstract away the protocol differences since the caller could just deal with the tid, knowing that the pid (if present) has already been checked.
- I wouldn't add `multiprocess` to the qSupported packets just yet. Since this is accepting the extended syntax regardless of whether the other side claimed to support it (there's no hard in doing that, right?), we could flip the switch after all packets have been ported.



================
Comment at: lldb/source/Utility/StringExtractor.cpp:375
+std::pair<llvm::Optional<lldb::pid_t>, llvm::Optional<lldb::tid_t>>
+StringExtractor::GetPidTid() {
+  llvm::Optional<lldb::pid_t> pid = llvm::None;
----------------
I think this would benefit from using more StringRefs (this class is ancient, so it reads very C like), similar to the `GetNameColonValue` function above. Maybe something like:
```
llvm::StringRef view = StringRef(m_packet).substr(m_index);
view = view.ltrim();
if (view.consume_front("p")) {
  view = view.ltrim();
  if (view.consume_front("-1") ...
    
}
```


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

https://reviews.llvm.org/D98482