refactor: Do not discard try_lock() return value #26189

1. 
   hebasto commented at 9:35 pm on September 27, 2022: member

   Microsoft’s C++ Standard Library uses the [[nodiscard]] attribute for try_lock(). See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex

   This change allows to drop the current suppression for the warning C4838 and helps to prevent the upcoming warning C4858. See: https://github.com/microsoft/STL/commit/539c26c923b38cd0b5eba2bb11de4bea9d5c6e43

   Fixes bitcoin/bitcoin#26017.

   Split from bitcoin/bitcoin#25819.

2. refactor: Do not discard `try_lock()` return value

   Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for
   `try_lock()`.
   See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex
   
   This change allows to drop the current suppression for the warning C4838
   and helps to prevent the upcoming warning C4858.
   See: https://github.com/microsoft/STL/commit/539c26c923b38cd0b5eba2bb11de4bea9d5c6e43

   bff4e068b6
3. 
   hebasto commented at 9:38 pm on September 27, 2022: member

   Friendly ping @ajtowns @ryanofsky @vasild
4. 
   ajtowns commented at 9:53 pm on September 27, 2022: contributor

   Concept ACK.

   This seems to have been introduced in 712fd182b72b0f5a1bcf843f171c29ec0a49b50f (April 2012) by @sipa – the old code for TryEnter did result = mutex.try_lock(); if (!result) pop_lock(); return result; the new code discarded the result for try_lock() and instead then checked !lock.owns().

5. 
   DrahtBot commented at 10:28 pm on September 27, 2022: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25081 (tracing: lock contention analysis by martinus)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

6. DrahtBot added the label Refactoring on Sep 28, 2022
7. vasild approved
8. 
   vasild commented at 8:37 am on September 28, 2022: contributor

   ACK bff4e068b69edd40a00466156f860bde2df29268

   The old and new code should have identical behavior. The new code is shorter and silences a warning.

   https://en.cppreference.com/w/cpp/thread/unique_lock/try_lock https://en.cppreference.com/w/cpp/thread/unique_lock/owns_lock

   The other call to Base::owns_lock() can also be avoided:

   0        EnterCritical(pszName, pszFile, nLine, Base::mutex(), true);
   1        if (!Base::try_lock()) {
   2            LeaveCritical();
   3            return false;
   4        }     
   5        return true;
   

9. fanquake requested review from sipa on Oct 2, 2022
10. 
    sipa commented at 8:41 pm on October 2, 2022: member

    ACK bff4e068b69edd40a00466156f860bde2df29268

    I’d suggest incorporating @vasild’s suggestion above.

11. fanquake added the label Waiting for author on Oct 3, 2022
12. refactor: Drop `owns_lock()` call

    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

    30cc1c6609
13. 
    hebasto commented at 11:31 am on October 3, 2022: member

    Updated bff4e068b69edd40a00466156f860bde2df29268 -> 30cc1c6609ad7868f73e88afe0b0233d395ec08c (pr26189.01 -> pr26189.02, diff):

    • added a commit which implements @vasild’s suggestion
14. hebasto removed the label Waiting for author on Oct 3, 2022
15. vasild approved
16. 
    vasild commented at 11:58 am on October 3, 2022: contributor

    ACK 30cc1c6609ad7868f73e88afe0b0233d395ec08c
17. fanquake merged this on Oct 3, 2022
18. fanquake closed this on Oct 3, 2022

    ---

19. sidhujag referenced this in commit 861db05f8b on Oct 4, 2022
20. fanquake referenced this in commit d919e8d574 on Oct 4, 2022
21. sidhujag referenced this in commit 012f9a05f1 on Oct 5, 2022
22. Fabcien referenced this in commit 6b2f9a78f4 on Jun 7, 2023
23. bitcoin locked this on Oct 3, 2023

Contributors
hebasto ajtowns DrahtBot vasild sipa

Labels
Refactoring