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

[Pavel Labath via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp:56
-        response,
-        std::chrono::duration_cast<std::chrono::microseconds>(kInterruptTimeout)
-            .count(),
----------------
zturner wrote:
> 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.
I am fine with that. Should we make that an official policy somewhere?


================
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);
----------------
zturner wrote:
> 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);`
I like literals as well, but I wasn't sure what is the general opinion about them.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h:83
   // created ScopedTimeout object got out of scope
   class ScopedTimeout {
   public:
----------------
zturner wrote:
> Could this class be templated on duration type?  So we could use us, ms, or whatever pleased us?
You don't need templates for this. Thanks to implicit conversions it can be used with any type with a coarser granularity than microseconds. It's only when Optional comes into the picture that things start to break down.


================
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);
----------------
zturner wrote:
> 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);
> ```
That sounds fine, although I would put the class in some place more general, as I plan to use it outside the gdb plugin as well.


https://reviews.llvm.org/D26971