jingham added a comment. 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. 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. ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:514-515 - llvm::Expected<TraceSupportedResponse> SendTraceSupported(); + llvm::Expected<TraceSupportedResponse> + SendTraceSupported(std::chrono::seconds interrupt_timeout); ---------------- clayborg wrote: > We have a ton of new "std::chrono::seconds interrupt_timeout" parameters. > Since GDBRemoteClientBase already has an ivar for this, can't we just use > this and pass a bool or no param instead? Note, m_interrupt_timeout is not an ivar of GDBRemoteClientBase, it's an ivar of GDBRemoteClientBase::Lock. 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
