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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits