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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
