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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
