amccarth added inline comments.
================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:397
+ if (slot != LLDB_INVALID_INDEX32) {
+ int id = m_watchpoint_slots[slot];
+ LLDB_LOG(log,
----------------
The names here are a bit confusing: `GetTriggeredHardwareBreakpointSlot`
doesn't return a slot; it returns the index of a slot, so `slot` also isn't a
slot, but the index of a slot. `m_watchpoint_slots` is not a collection of
slots but IDs, that's indexed by an index called a `slot`.
It may not be possible to completely rationalize this, but doing even a little
could help future readers understand. For example, `slot` could be
`slot_index`, and `m_watchpoint_slots` could be `m_watchpoint_ids` (or even
just `m_watchpoints`).
================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:525
for (const auto &thread_info : m_session_data->m_new_threads) {
- ThreadSP thread(new TargetThreadWindows(*this, thread_info.second));
- thread->SetID(thread_info.first);
- new_thread_list.AddThread(thread);
+ new_thread_list.AddThread(thread_info.second);
++new_size;
----------------
This no longer sets the thread ID. Was that intentional?
================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:743
-void ProcessWindows::OnCreateThread(const HostThread &new_thread) {
+void ProcessWindows::OnCreateThread(HostThread &new_thread) {
llvm::sys::ScopedLock lock(m_mutex);
----------------
Can you help me why we needed to get rid of the `const` on the `HostThread &`?
If `native_new_thread` were also a const ref, I don't see any conflicting
constraint.
================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:832
+ Status error;
+
+ WatchpointInfo info;
----------------
Should we check if the watchpoint is already enabled? I noticed that
`DisableWatchpoint` has an analogous guard clause.
================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:852
+ RegisterContextWindows *reg_ctx = static_cast<RegisterContextWindows *>(
+ thread->GetRegisterContext().get());
+ if (!reg_ctx->AddHardwareBreakpoint(info.slot, info.address, info.size,
----------------
Since you have to explicitly downcast, wouldn't `auto *reg_ctx` on the left be
sufficient and just as clear?
================
Comment at:
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:82
-bool RegisterContextWindows::ClearHardwareBreakpoint(uint32_t hw_idx) {
- return false;
-}
+ if (!size || size > 8 || size & (size - 1))
+ return false;
----------------
Clever! It took me a minute or two to figure out what the point of that was
checking. Perhaps a comment to explain?
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67168/new/
https://reviews.llvm.org/D67168
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
