mib added inline comments.
================ Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:386 + // to add 1 to its return value. + return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1; + } ---------------- JDevlieghere wrote: > Sorry if my previous comment wasn't clear. `std::atomic` provides these > operators so the old code was fine: > https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith Makes sense, we shouldn't touch the implementation of `IncrementLockCount`. ================ Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:389-398 uint32_t DecrementLockCount() { - if (m_lock_count > 0) - --m_lock_count; - return m_lock_count; + // std::atomic::fetch_sub is an atomic post-decrement operation so we need + // to subtract 1 from its return value. + uint32_t count = m_lock_count.fetch_sub(1, std::memory_order_relaxed) - 1; + if (static_cast<int32_t>(count) < 0) { + // Handle underflow here & reset count to zero. + count = 0; ---------------- You're right, but I think we should at least keep the guardrails to prevent returning an underflow value and return 0 instead. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154271/new/ https://reviews.llvm.org/D154271 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
