[Lldb-commits] [PATCH] D25391: Remove usages of TimeValue from gdb-remote process plugin

[Pavel Labath via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:192
+    packet_result = ReadPacket(
+        response, std::chrono::duration_cast<std::chrono::microseconds>(
+                      GetPacketTimeout())
----------------
zturner wrote:
> `using namespace std` is generally frowned upon, but I wonder if we could be more lenient about `using namespace std::chrono`?  Would make a lot of those code easier on the eyes.
I started using it in some tighter scopes, which are chrono-heavy. I'd be fine with the using directive at file level though.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:266-267
+      packet,
+      std::chrono::duration_cast<std::chrono::microseconds>(GetPacketTimeout())
+          .count(),
+      false);
----------------
zturner wrote:
> Same here.  You should just be able to pass the `duration` straight through, and update `ReadPacket` to do the cast.
These should go away, when everything is updated to use chrono, but the hierarchy there is multiple levels deep, so I cannot do it in a single pass (ReadPacket just calls `WaitForPacketWithTimeoutMicroSecondsNoLock`, which would also need to be updated... the real cast should only happen somewhere in the Connection classes, once you actually start issuing system calls).


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h:86
+    ScopedTimeout(GDBRemoteCommunication &gdb_comm,
+                  std::chrono::seconds timeout);
     ~ScopedTimeout();
----------------
zturner wrote:
> To make this more generic, this could be a `std::chrono::duration<int>`.  If for any reason someone wanted `3.2` seconds, they could then specify it.
I don't think that works the way you think it does. If you don't specify the ratio template argument, it will just get defaulted to `std::ratio<1>`, meaning seconds.  To have it generic you'd have to make this a template function, with the ratio as the parameter, but at that point you might as well just choose a type with the highest precision you are willing to accept. So, I could make this take a milli or micro second durations, but I don't think that's necessary for a packet timeout.


https://reviews.llvm.org/D25391