clayborg added a comment. In D102085#2745450 <https://reviews.llvm.org/D102085#2745450>, @jingham wrote:
> I don't see the change as "adding a bunch of timeouts", since what it mostly > did was remove a bunch of send_async = false's. For all the clients that > don't care about a timeout, I removed the send_async parameter, and for those > that did I replaced it with a timeout. There were a few cases at the > GDBRemoteClientBase layer where routines were calling the SendPacket routines > with send_async = true w/o requesting that info from ProcessGDBRemote. To > those I added a timeout parameter. That I consider a plus since it made > explicit which packet request functions were interrupting and which were not. > > Given the packet requests that called with send_async = false made up ~95% > of all uses, it seemed like marking the few uses that did want to interrupt > by the fact that they explicitly receive a timeout parameter was cleaner than > switching everybody over to a Timeout, and then having 95% of the clients > still have to make up an empty Timeout. > > Instead, the rule is now "client functions that are going to interrupt the > target get passed a timeout, and otherwise you don't have to worry about it". > That removed a bunch of boiler-plate and seems clear to me. > > We could go further, as you suggest, and add an m_interrupt_timeout to > GDBRemoteClientBase (*). That would avoid ever having to pass a timeout into > the SendPacket client functions in GDBRemoteClientBase. We already have > ValueChanged callbacks for Properties. We'd have to add a virtual > "InterruptTimeoutChanged" to Process to do the right thing for > ProcessGDBRemote. None of that would be hard to do. > > In fact, I actually thought about doing that, but I didn't because I like the > fact that from ProcessGDBRemote, you are able to tell which methods in > GDBRemoteClientBase interrupt a running process, and which don't by whether > they take a timeout or not. If the timeout were in the GDBRemoteClientBase > class you wouldn't see that. But that does seems a thing worth knowing. We > could preserve that knowledge by adding some naming convention to distinguish > between interrupting & non-interrupting methods. But I don't see what that > really gains us. I just note when I see a ton of changes to things that can easily be fixed by adding an ivar I tend to do it. It is fine that the "bool async" parameter went away. We should probably combine the two functions that only differ by one taking a timeout and the other not. They are very similar and can easily use a Optional<> on the timeout parameter. > OTOH, if the general consensus is that is isn't important to know whether an > information requesting function in GDBRemoteClientBase is interrupting or > not, I can certainly add the variable and get it to track the setting changes > in Process & ProcessGDBRemote. > > (*) We would have to add it, the one that's already there belongs to the > GDBRemoteClientBase::Lock class, which is short-lived, not to the > GDBRemoteClientBase per se. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102085/new/ https://reviews.llvm.org/D102085 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
