[Lldb-commits] [PATCH] D26971: Introduce chrono to more gdb-remote functions

[Zachary Turner via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:56
-        response,
-        std::chrono::duration_cast<std::chrono::microseconds>(kInterruptTimeout)
-            .count(),
----------------
I think we should all be willing to agree that `using namespace std::chrono` is fine in CPP files.  The namespace is not particularly large, and it makes code much nicer to read.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:249
   StringExtractorGDBRemote extra_stop_reply_packet;
-  uint32_t timeout_usec = 100000; // 100ms
-  ReadPacket(extra_stop_reply_packet, timeout_usec, false);
+  std::chrono::microseconds timeout(100000); // 100ms
+  ReadPacket(extra_stop_reply_packet, timeout, false);
----------------
You can also write `using namespace std::chrono::literals` at the top of the file, and then you can write:

`ReadPacket(extra_stop_reply_packet, 100ms, false);`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:136
 #ifdef LLDB_CONFIGURATION_DEBUG
-      m_packet_timeout(1000),
+      m_packet_timeout(1000000000),
 #else
----------------
clayborg wrote:
> Can me make this clearer with something like:
> ```
> #ifdef LLDB_CONFIGURATION_DEBUG
>     m_packet_timeout(std::chrono::seconds(1000));
> #else
>     m_packet_timeout(std::chrono::seconds(1));
> #endif
> ```
Or if using literals, `1000s` or `1s`


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h:83
   // created ScopedTimeout object got out of scope
   class ScopedTimeout {
   public:
----------------
Could this class be templated on duration type?  So we could use us, ms, or whatever pleased us?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h:232
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
-                          uint32_t timeout_usec, bool sync_on_timeout);
+                          llvm::Optional<std::chrono::microseconds> timeout,
+                          bool sync_on_timeout);
----------------
One idea might be to have 

```
template <typename T>
using GdbTimeout = llvm::Optional<std::chrono::duration<int, T>
```

then you could write:

```
PacketResult ReadPacket(StringExtractorGDBRemote &response, GdbTimeout<std::micro> timeout);
```


https://reviews.llvm.org/D26971