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

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

In D98482#2624973 <https://reviews.llvm.org/D98482#2624973>, @labath wrote:

> I'm still bugged by the GetPidTid interface -- the pair of optionals is very complicated. What if we had this function take a `default_pid` argument (which would be equal to the currently selected process), and it returned that as the process id, in case the packet does not contain one? Then the interface could be something like `Optional<pair<pid_t, tid_t>> GetPidTid(pid_t default_pid)` (because tid automatically defaults to -1), which I think is more intuitive. In case we really need to differentiate between a pid not being present and an implicitly chosen pid, then the interface could be like `Optional<pair<Optional<pid_t>, tid_t>> GetPidTid()`, which still seems /slightly/ better as its easier to recognise invalid input. WDYT?

I've originally wanted to precisely convey whatever was in the packet since it was a generic class, and I actually thought nested `Optional`s are going to be worse. But thinking about it, you're probably right — the spec explicitly says to assume `tid=-1` when only pid is provided, so I guess we can use a single `Optional` and just assume a default pid. However, I think I'm going to go a step further and eliminate `pair` as well, in favor of `llvm::Optional<lldb::tid_t> GetPidTid(lldb:pid_t& pid)`, with `pid` being modified if specified or otherwise left in its current (i.e. default) value.


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

https://reviews.llvm.org/D98482