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

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

It would be nice if we can find a way to be able to use any time units we want when specifying the timeout. I would rather not have everyone have to convert their times to microseconds. Maybe we just add some overloads to the public facing functions and have just one internal function that uses llvm::Optional.

As a side note I would also like us to fix llvm::Optional so it can be debugged. Right now if you expand an llvm::Optional<T> you see the an array of characters instead of your type T which means when debugging you can't see your type's value. Boost correctly uses C++11 unions so that we can see the type when expanded in the debugger.



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:136
 #ifdef LLDB_CONFIGURATION_DEBUG
-      m_packet_timeout(1000),
+      m_packet_timeout(1000000000),
 #else
----------------
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
```


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:125
     PacketResult packet_result = PacketResult::Success;
-    const uint32_t timeout_usec = 10 * 1000; // Wait for 10 ms for a response
+    const std::chrono::microseconds timeout(10000); // 10 ms
     while (packet_result == PacketResult::Success)
----------------
Can't we do:

```
const std::chrono::milliseconds timeout(10);
```

Or is this what you mentioned with the llvm::Optional stuff in your description?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:206
+    ScopedTimeout timeout(*this, std::max(GetPacketTimeout(),
+                                          std::chrono::microseconds(6000000)));
 
----------------
It would be nice if we can just use std::chrono::seconds. I would hope this line doesn't need to change? 


https://reviews.llvm.org/D26971