labath added a comment. In D98482#2626237 <https://reviews.llvm.org/D98482#2626237>, @mgorny wrote:
> Create a new `GDBRemoteError` class to pass gdb-remote `$E` codes through > cleanly. The error codes we use right now are completely meaningless, so don't bother preserving them. I don't think we should be introducing a separate error class on their account, and I'm particularly unhappy about how this class has insinuated itself into the Status object. In D98482#2631420 <https://reviews.llvm.org/D98482#2631420>, @mgorny wrote: > I've just discovered that `LLDB_INVALID_*_ID` isn't -1/UINT_MAX as I thought, > so I've had to introduce additional constants. That said, I'm not convinced > about the current 0/-1 API. Maybe it'd make sense to handle them directly in > `ReadTid()` and return some (current?) thread ID for 0, and > `LLDB_INVALID_*_ID` for -1? I think that decision which thread to use for the "any" thread should not be handled at such a low level, as it can have real impact on how the inferior behaves. I think using constants for this is fine. Although I applaud the avoidance of macros, I think the constants should also use non-macro nomenclature. Maybe just put the names inside the StringExtractorGdbRemote class, and call them `AnyThread` (etc.) ? Also, I think that these days we can just use constexpr variables instead of the enum hack. ================ Comment at: lldb/source/Utility/StringExtractorGDBRemote.cpp:625 + uint64_t prev_index = m_index; + pid = GetHexMaxU64(false, 0); + if (m_index == prev_index || m_index == UINT64_MAX) { ---------------- This should be something like `view.consumeInteger(16, pid)`. At that point you can stop keeping m_index in sync, and just update it at the very end (maybe even via llvm::scope_exit). ================ Comment at: lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp:8-10 +namespace { +class StringExtractorGDBRemoteTest : public ::testing::Test {}; +} // namespace ---------------- For empty fixtures, you can just drop this, along with the _F in the TEST_F macro. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98482/new/ https://reviews.llvm.org/D98482 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
