Repository: microsoft/terminal
File: src/cascadia/Remoting/Monarch.cpp (C++)

Code before:
        {
            // TODO:projects/5 This is terrible. There's gotta be a better way
            // of finding the first opening in a non-consecutive map of int->object
            const auto providedID = peasant.GetID();

            if (providedID == 0)
            {
                // Peasant doesn't currently have an ID. Assign it a new one.
                peasant.AssignID(_nextPeasantID++);
            }
            else
            {
                uint64_t old = 0;

                // Peasant already had an ID (from an older monarch). Leave that one
                // be. Make sure that the next peasant's ID is higher than it.
                // If multiple peasants are added concurrently we keep trying to update
                // until we get to set the new id.
                while (!_nextPeasantID.compare_exchange_strong(old, providedID + 1))
                {
                    old = _nextPeasantID.load();
                    if (providedID < old)
                    {
                        break;
                    }
                }
            }

            auto newPeasantsId = peasant.GetID();

            // Keep track of which peasant we are
            // SAFETY: this is only true for one peasant, and each peasant
            // is only added to a monarch once, so we do not need synchronization here.
            if (peasant.GetPID() == _ourPID)
            {
                _ourPeasantId = newPeasantsId;
            }
            // Add an event listener to the peasant's WindowActivated event.
            peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated });
            peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows });
            peasant.RenameRequested({ this, &Monarch::_renameRequested });

            peasant.ShowTrayIconRequested([this](auto&&, auto&&) { _ShowTrayIconRequestedHandlers(*this, nullptr); });
            peasant.HideTrayIconRequested([this](auto&&, auto&&) { _HideTrayIconRequestedHandlers(*this, nullptr); });
            peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll });

            {
                std::unique_lock lock{ _peasantsMutex };
                _peasants[newPeasantsId] = peasant;
            }


Review comment (suggestion):
Strong exchanges aren't needed here as you're looping anways. By using a do-while loop we can get rid of the if condition. This results in:
```cpp
uint64_t current;
do
{
    current = _nextPeasantID.load(std::memory_order_relaxed);
}
while (current <= providedID && !_nextPeasantID.compare_exchange_weak(current, providedID + 1, std::memory_order_relaxed));
```

Code after:
        try
        {
            // TODO:projects/5 This is terrible. There's gotta be a better way
            // of finding the first opening in a non-consecutive map of int->object
            const auto providedID = peasant.GetID();

            if (providedID == 0)
            {
                // Peasant doesn't currently have an ID. Assign it a new one.
                peasant.AssignID(_nextPeasantID++);
            }
            else
            {
                // Peasant already had an ID (from an older monarch). Leave that one
                // be. Make sure that the next peasant's ID is higher than it.
                // If multiple peasants are added concurrently we keep trying to update
                // until we get to set the new id.
                uint64_t current;
                do
                {
                    current = _nextPeasantID.load(std::memory_order_relaxed);
                } while (current <= providedID && !_nextPeasantID.compare_exchange_weak(current, providedID + 1, std::memory_order_relaxed));
            }

            auto newPeasantsId = peasant.GetID();

            // Keep track of which peasant we are
            // SAFETY: this is only true for one peasant, and each peasant
            // is only added to a monarch once, so we do not need synchronization here.
            if (peasant.GetPID() == _ourPID)
            {
                _ourPeasantId = newPeasantsId;
            }
            // Add an event listener to the peasant's WindowActivated event.
            peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated });
            peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows });
            peasant.RenameRequested({ this, &Monarch::_renameRequested });

            peasant.ShowNotificationIconRequested([this](auto&&, auto&&) { _ShowNotificationIconRequestedHandlers(*this, nullptr); });
            peasant.HideNotificationIconRequested([this](auto&&, auto&&) { _HideNotificationIconRequestedHandlers(*this, nullptr); });
            peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll });

            {
                std::unique_lock lock{ _peasantsMutex };
                _peasants[newPeasantsId] = peasant;
            }

            TraceLoggingWrite(g_hRemotingProvider,
                              "Monarch_AddPeasant",
                              TraceLoggingUInt64(providedID, "providedID", "the provided ID for the peasant"),
                              TraceLoggingUInt64(newPeasantsId, "peasantID", "the ID of the new peasant"),