ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/renderer/uia/UiaRenderer.cpp (C++)

Code before:
// RenderEngineBase defines a WaitUntilCanRender() that sleeps for 8ms to throttle rendering.
// But UiaEngine is never the only engine running. Overriding this function prevents
// us from sleeping 16ms per frame, when the other engine also sleeps for 8ms.
void UiaEngine::WaitUntilCanRender() noexcept
{
}

// Routine Description:
// - Used to perform longer running presentation steps outside the lock so the
//      other threads can continue.
// - Not currently used by UiaEngine.
// Arguments:
// - <none>
// Return Value:
// - S_FALSE since we do nothing.
[[nodiscard]] HRESULT UiaEngine::Present() noexcept
{
    RETURN_HR_IF(S_FALSE, !_isEnabled);
    RETURN_HR_IF(E_INVALIDARG, !_isPainting); // invalid to end paint when we're not painting

    // Fire UIA Events here
    if (_selectionChanged)
    {
        try
        {
            _dispatcher->SignalSelectionChanged();
        }
        CATCH_LOG();
    }
    if (_textBufferChanged)
    {
        try
        {
            _dispatcher->SignalTextChanged();
        }
        CATCH_LOG();
    }
    if (_cursorChanged)
    {
        try
        {
            _dispatcher->SignalCursorChanged();
        }
        CATCH_LOG();
    }
    if (!_newOutput.empty())
    {
        try
        {
            _dispatcher->NotifyNewOutput(_newOutput);
        }

Review comment (suggestion):
The diff is weird here. I basically just moved all of the contents of `EndPaint()` over to `Present()`. This way we're not doing this under lock. Leonard said that's the main difference between the two functions.

Code after:

    // Snap this now while we're still under lock
    // so present can work on the copy while another
    // thread might start filling the next "frame"
    // worth of text data.
    std::swap(_queuedOutput, _newOutput);
    _newOutput.clear();
    return S_OK;
}

// RenderEngineBase defines a WaitUntilCanRender() that sleeps for 8ms to throttle rendering.
// But UiaEngine is never the only engine running. Overriding this function prevents
// us from sleeping 16ms per frame, when the other engine also sleeps for 8ms.
void UiaEngine::WaitUntilCanRender() noexcept
{
}

// Routine Description:
// - Used to perform longer running presentation steps outside the lock so the
//      other threads can continue.
// - Not currently used by UiaEngine.
// Arguments:
// - <none>
// Return Value:
// - S_FALSE since we do nothing.
[[nodiscard]] HRESULT UiaEngine::Present() noexcept
{
    RETURN_HR_IF(S_FALSE, !_isEnabled);

    // Fire UIA Events here
    if (_selectionChanged)
    {
        try
        {
            _dispatcher->SignalSelectionChanged();
        }
        CATCH_LOG();
    }
    if (_textBufferChanged)
    {
        try
        {
            _dispatcher->SignalTextChanged();
        }
        CATCH_LOG();
    }
    if (_cursorChanged)
    {
        try
        {
            _dispatcher->SignalCursorChanged();
Repository: microsoft/terminal
File: src/renderer/uia/UiaRenderer.cpp (C++)

Code before:
[[nodiscard]] HRESULT UiaEngine::InvalidateAll() noexcept
{
    _textBufferChanged = true;
    return S_OK;
}

// Routine Description:
// - This currently has no effect in this renderer.
// Arguments:
// - pForcePaint - Always filled with false
// Return Value:
// - S_FALSE because we don't use this.
[[nodiscard]] HRESULT UiaEngine::InvalidateCircling(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

[[nodiscard]] HRESULT UiaEngine::NotifyNewText(const std::wstring_view newText) noexcept
try
{
    if (!newText.empty())
    {
        _newOutput += newText;
    }
    return S_OK;
}
CATCH_LOG_RETURN_HR(E_FAIL);

// Routine Description:
// - This is unused by this renderer.
// Arguments:
// - pForcePaint - always filled with false.
// Return Value:
// - S_FALSE because this is unused.
[[nodiscard]] HRESULT UiaEngine::PrepareForTeardown(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

// Routine Description:
// - Prepares internal structures for a painting operation.
// Arguments:
// - <none>
// Return Value:
// - S_OK if we started to paint. S_FALSE if we didn't need to paint.

Review comment (suggestion):
iirc this is not the most performant operation, but it's probably okay because we only hit this one when we already have UIA enabled, and at that point we're already off the optimal path.

I coulda swore the `VtRenderer` did something clever for building up its buffer, but meh

Code after:
[[nodiscard]] HRESULT UiaEngine::InvalidateAll() noexcept
{
    _textBufferChanged = true;
    return S_OK;
}

// Routine Description:
// - This currently has no effect in this renderer.
// Arguments:
// - pForcePaint - Always filled with false
// Return Value:
// - S_FALSE because we don't use this.
[[nodiscard]] HRESULT UiaEngine::InvalidateCircling(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

[[nodiscard]] HRESULT UiaEngine::NotifyNewText(const std::wstring_view newText) noexcept
try
{
    if (!newText.empty())
    {
        _newOutput.append(newText);
        _newOutput.push_back(L'\n');
    }
    return S_OK;
}
CATCH_LOG_RETURN_HR(E_FAIL);

// Routine Description:
// - This is unused by this renderer.
// Arguments:
// - pForcePaint - always filled with false.
// Return Value:
// - S_FALSE because this is unused.
[[nodiscard]] HRESULT UiaEngine::PrepareForTeardown(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

// Routine Description:
// - Prepares internal structures for a painting operation.
// Arguments:
// - <none>
// Return Value:
Repository: microsoft/terminal
File: src/renderer/uia/UiaRenderer.cpp (C++)

Code before:
    return S_OK;
}

// Routine Description:
// - This currently has no effect in this renderer.
// Arguments:
// - pForcePaint - Always filled with false
// Return Value:
// - S_FALSE because we don't use this.
[[nodiscard]] HRESULT UiaEngine::InvalidateCircling(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

[[nodiscard]] HRESULT UiaEngine::NotifyNewText(const std::wstring_view newText) noexcept
try
{
    if (!newText.empty())
    {
        static constexpr std::wstring_view newline{ L"\n" };
        _newOutput.reserve(_newOutput.size() + newText.size() + 1);
        _newOutput.append(newText);
        _newOutput.append(newline);
    }
    return S_OK;
}
CATCH_LOG_RETURN_HR(E_FAIL);

// Routine Description:
// - This is unused by this renderer.
// Arguments:
// - pForcePaint - always filled with false.
// Return Value:
// - S_FALSE because this is unused.
[[nodiscard]] HRESULT UiaEngine::PrepareForTeardown(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

// Routine Description:
// - Prepares internal structures for a painting operation.
// Arguments:
// - <none>
// Return Value:
// - S_OK if we started to paint. S_FALSE if we didn't need to paint.

Review comment (suggestion):
IMO this is fine:
```cpp
_newOutput.append(newText);
_newOutput.push_back(L'\n');
```

Normally I'm a sucker for preallocating memory, but in this case the memory allocation will be amortized over time due to our `std::swap` trick. So in general, after a while, this here will practically never reallocate again.

Code after:
    return S_OK;
}

// Routine Description:
// - This currently has no effect in this renderer.
// Arguments:
// - pForcePaint - Always filled with false
// Return Value:
// - S_FALSE because we don't use this.
[[nodiscard]] HRESULT UiaEngine::InvalidateCircling(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

[[nodiscard]] HRESULT UiaEngine::NotifyNewText(const std::wstring_view newText) noexcept
try
{
    if (!newText.empty())
    {
        _newOutput.append(newText);
        _newOutput.push_back(L'\n');
    }
    return S_OK;
}
CATCH_LOG_RETURN_HR(E_FAIL);

// Routine Description:
// - This is unused by this renderer.
// Arguments:
// - pForcePaint - always filled with false.
// Return Value:
// - S_FALSE because this is unused.
[[nodiscard]] HRESULT UiaEngine::PrepareForTeardown(_Out_ bool* const pForcePaint) noexcept
{
    RETURN_HR_IF_NULL(E_INVALIDARG, pForcePaint);

    *pForcePaint = false;
    return S_FALSE;
}

// Routine Description:
// - Prepares internal structures for a painting operation.
// Arguments:
// - <none>
// Return Value:
// - S_OK if we started to paint. S_FALSE if we didn't need to paint.
[[nodiscard]] HRESULT UiaEngine::StartPaint() noexcept
{
Repository: microsoft/terminal
File: src/types/utils.cpp (C++)

Code before:
                // In that case, manually mangle the startingDirectory to use
                // backslashes as the path separator instead.
                std::wstring mangledDirectory{ startingDirectory };
                if (til::starts_with(mangledDirectory, L"//wsl$") || til::starts_with(mangledDirectory, L"//wsl.localhost"))
                {
                    mangledDirectory = std::filesystem::path{ startingDirectory }.make_preferred().wstring();
                }

                return {
                    fmt::format(LR"("{}" --cd "{}" {})", executablePath.wstring(), mangledDirectory, arguments),
                    std::wstring{}
                };
            }
        }
    } while (false);

    // GH #12353: `~` is never a valid windows path. We can only accept that as
    // a startingDirectory when the exe is specifically wsl.exe, because that
    // can override the real startingDirectory. If the user set the
    // startingDirectory to ~, but the commandline to something like pwsh.exe,
    // that won't actually work. In that case, mangle the startingDirectory to
    // %userprofile%, so it's at least something reasonable.
    return {
        std::wstring{ commandLine },
        std::wstring{ startingDirectory == L"~" ? wil::ExpandEnvironmentStringsW<std::wstring>(L"%USERPROFILE%") :
                                                  startingDirectory }
    };
}


Review comment (suggestion):
```suggestion
        startingDirectory == L"~" ? wil::ExpandEnvironmentStringsW<std::wstring>(L"%USERPROFILE%") :
                                    std::wstring{ startingDirectory }
```

Code after:
                // In that case, manually mangle the startingDirectory to use
                // backslashes as the path separator instead.
                std::wstring mangledDirectory{ startingDirectory };
                if (til::starts_with(mangledDirectory, L"//wsl$") || til::starts_with(mangledDirectory, L"//wsl.localhost"))
                {
                    mangledDirectory = std::filesystem::path{ startingDirectory }.make_preferred().wstring();
                }

                return {
                    fmt::format(LR"("{}" --cd "{}" {})", executablePath.wstring(), mangledDirectory, arguments),
                    std::wstring{}
                };
            }
        }
    } while (false);

    // GH #12353: `~` is never a valid windows path. We can only accept that as
    // a startingDirectory when the exe is specifically wsl.exe, because that
    // can override the real startingDirectory. If the user set the
    // startingDirectory to ~, but the commandline to something like pwsh.exe,
    // that won't actually work. In that case, mangle the startingDirectory to
    // %userprofile%, so it's at least something reasonable.
    return {
        std::wstring{ commandLine },
        startingDirectory == L"~" ? wil::ExpandEnvironmentStringsW<std::wstring>(L"%USERPROFILE%") :
                                    std::wstring{ startingDirectory }
    };
}
Repository: microsoft/terminal
File: src/terminal/adapter/InteractDispatch.cpp (C++)

Code before:

                        // Get the PID of the current FG window, and compare with our owner's PID.
                        DWORD currentFgPid{ 0 };
                        DWORD ownerPid{ 0 };
                        GetWindowThreadProcessId(currentFgWindow, &currentFgPid);
                        GetWindowThreadProcessId(ownerHwnd, &ownerPid);

                        if (ownerPid == currentFgPid)
                        {
                            // Huzzah, the app that owns us is actually the FG
                            // process. They're allowed to grand FG rights.
                            shouldActuallyFocus = true;
                        }
                    }
                }
            }
        }

        WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, shouldActuallyFocus);
        gci.ProcessHandleList.ModifyConsoleProcessFocus(shouldActuallyFocus);
    }
    // Does nothing outside of ConPTY. If there's a real HWND, then the HWND is solely in charge.

    return true;
}


Review comment (suggestion):
we should make sure that a VT Input client on the far receiving end doesn't get _two events each focus change_. I was wondering if it would because:

1. we might pass one through (original one)
2. we add a `FOCUS_EVENT` to the input buffer, which TerminalInput later converts into another focus VT sequence

Code after:
                        // isn't one)

                        // Get the PID of the current FG window, and compare with our owner's PID.
                        DWORD currentFgPid{ 0 };
                        DWORD ownerPid{ 0 };
                        GetWindowThreadProcessId(currentFgWindow, &currentFgPid);
                        GetWindowThreadProcessId(ownerHwnd, &ownerPid);

                        if (ownerPid == currentFgPid)
                        {
                            // Huzzah, the app that owns us is actually the FG
                            // process. They're allowed to grand FG rights.
                            shouldActuallyFocus = true;
                        }
                    }
                }
            }
        }

        WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, shouldActuallyFocus);
        gci.ProcessHandleList.ModifyConsoleProcessFocus(shouldActuallyFocus);
        gci.pInputBuffer->Write(std::make_unique<FocusEvent>(focused));
    }
    // Does nothing outside of ConPTY. If there's a real HWND, then the HWND is solely in charge.

    return true;
}
Repository: microsoft/terminal
File: src/interactivity/base/InteractivityFactory.cpp (C++)

Code before:
            {
                _WritePseudoWindowCallback((bool)wParam);
            }
        }
    }
    // If we get this far, call the default window proc
    return DefWindowProcW(hWnd, Message, wParam, lParam);
}

// Method Description:
// - Helper for the pseudo console message loop to send a notification
//   when it realizes we should be showing or hiding the window.
// - Simply skips if no callback is installed.
// Arguments:
// - showOrHide: Show is true; hide is false.
// Return Value:
// - <none>
void InteractivityFactory::_WritePseudoWindowCallback(bool showOrHide)
{
    // BODGY
    //
    // GH#13158 - At least temporarily, only allow the PTY to HIDE the terminal
    // window. There seem to be many issues with this so far, and the quickest
    // route to mitigate them seems to be limiting the interaction here to
    // allowing ConPTY to minimize the terminal only. This will still allow
    // bodgy scripts to hide the Terminal via GetConsoleWindow(), but should
    // broadly prevent any other impact of this feature.
    //
    // Should we need to restore this functionality in the future, we should
    // only do so with great caution.
    if (_pseudoWindowMessageCallback && showOrHide == false)
    {
        _pseudoWindowMessageCallback(showOrHide);
    }
}

#pragma endregion


Review comment (suggestion):
```suggestion
    // applications to hide the Terminal via GetConsoleWindow(), but should
```

I am doing this because speel cheek didn't like it but mysteriously didn't block the build

Code after:
            {
                _WritePseudoWindowCallback((bool)wParam);
            }
        }
    }
    // If we get this far, call the default window proc
    return DefWindowProcW(hWnd, Message, wParam, lParam);
}

// Method Description:
// - Helper for the pseudo console message loop to send a notification
//   when it realizes we should be showing or hiding the window.
// - Simply skips if no callback is installed.
// Arguments:
// - showOrHide: Show is true; hide is false.
// Return Value:
// - <none>
void InteractivityFactory::_WritePseudoWindowCallback(bool showOrHide)
{
    // BODGY
    //
    // GH#13158 - At least temporarily, only allow the PTY to HIDE the terminal
    // window. There seem to be many issues with this so far, and the quickest
    // route to mitigate them seems to be limiting the interaction here to
    // allowing ConPTY to minimize the terminal only. This will still allow
    // applications to hide the Terminal via GetConsoleWindow(), but should
    // broadly prevent any other impact of this feature.
    //
    // Should we need to restore this functionality in the future, we should
    // only do so with great caution.
    if (_pseudoWindowMessageCallback && showOrHide == false)
    {
        _pseudoWindowMessageCallback(showOrHide);
    }
}

#pragma endregion
Repository: microsoft/terminal
File: src/types/inc/IInputEvent.hpp (C++)

Code before:

    ~FocusEvent();
    FocusEvent(const FocusEvent&) = default;
    FocusEvent(FocusEvent&&) = default;
    FocusEvent& operator=(const FocusEvent&) & = default;
    FocusEvent& operator=(FocusEvent&&) & = default;

    INPUT_RECORD ToInputRecord() const noexcept override;
    InputEventType EventType() const noexcept override;

    constexpr bool GetFocus() const noexcept
    {
        return _focus;
    }

    void SetFocus(const bool focus) noexcept;

    // BODGY - see FocusEvent.cpp for details.
    constexpr bool CameFromApi() const noexcept
    {
        return _cameFromApi;
    }

private:
    bool _focus;
    const bool _cameFromApi;

#ifdef UNIT_TESTING
    friend std::wostream& operator<<(std::wostream& stream, const FocusEvent* const pFocusEvent);
#endif
};

#ifdef UNIT_TESTING
std::wostream& operator<<(std::wostream& stream, const FocusEvent* const pFocusEvent);
#endif


Review comment (suggestion):
Generally, don't use `const` on members. It prevents `std::move` and other kinds of language/compiler features, because C++ is weird.

Code after:

    ~FocusEvent();
    FocusEvent(const FocusEvent&) = default;
    FocusEvent(FocusEvent&&) = default;
    FocusEvent& operator=(const FocusEvent&) & = default;
    FocusEvent& operator=(FocusEvent&&) & = default;

    INPUT_RECORD ToInputRecord() const noexcept override;
    InputEventType EventType() const noexcept override;

    constexpr bool GetFocus() const noexcept
    {
        return _focus;
    }

    void SetFocus(const bool focus) noexcept;

    // BODGY - see FocusEvent.cpp for details.
    constexpr bool CameFromApi() const noexcept
    {
        return _cameFromApi;
    }

private:
    bool _focus;
    bool _cameFromApi;

#ifdef UNIT_TESTING
    friend std::wostream& operator<<(std::wostream& stream, const FocusEvent* const pFocusEvent);
#endif
};

#ifdef UNIT_TESTING
std::wostream& operator<<(std::wostream& stream, const FocusEvent* const pFocusEvent);
#endif
Repository: microsoft/terminal
File: tools/bx.ps1 (PowerShell)

Code before:
# This is a helper script to figure out which target corresponds to the project
# in this directory. Parses the solution's .metaproj file looking for the
# project file in this directory, to be able to get the project's name.

$projects = Get-ChildItem .\ | where {$_.extension -in ".vcxproj",".wapproj"}
if ($projects.length -eq 0)
{
    exit -1
}
$projectPath = $projects.FullName


# Parse the solution's metaproj file.
[xml]$Metaproj = Get-Content "$env:OPENCON\OpenConsole.sln.metaproj"

$targets = $Metaproj.Project.Target

# Most projects are in OpenConsole.sln.metaproj as "<project>.*proj.metaproj".
# We'll filter to search for these first and foremost.
$msBuildCondition = "'%(ProjectReference.Identity)' == '$projectPath.metaproj'"

# Filter to project targets that match our metaproj file.
# For Conhost\Server, this will match:
#   [Conhost\Server, Conhost\Server:Clean, Conhost\Server:Rebuild, Conhost\Server:Publish]
$matchingTargets = $targets | Where-Object { $_.MSBuild.Condition -eq $msBuildCondition }

# If we didn't find a target, it's possible that the project didn't have a
# .metaproj in OpenConsole.sln.metaproj. Try filtering again, but leave off the
# .metaproj extension.
if ($matchingTargets.length -eq 0)

Review comment (suggestion):
```suggestion
$projects = Get-ChildItem -Path * -Include *.vcxproj,*.wapproj
```

Code after:
# This is a helper script to figure out which target corresponds to the project
# in this directory. Parses the solution's .metaproj file looking for the
# project file in this directory, to be able to get the project's name.

$projects = Get-ChildItem -Path * -Include *.vcxproj,*.wapproj
if ($projects.length -eq 0)
{
    exit -1
}
$projectPath = $projects.FullName


# Parse the solution's metaproj file.
[xml]$Metaproj = Get-Content "$env:OPENCON\OpenConsole.sln.metaproj"

$targets = $Metaproj.Project.Target

# Most projects are in OpenConsole.sln.metaproj as "<project>.*proj.metaproj".
# We'll filter to search for these first and foremost.
$msBuildCondition = "'%(ProjectReference.Identity)' == '$projectPath.metaproj'"

# Filter to project targets that match our metaproj file.
# For Conhost\Server, this will match:
#   [Conhost\Server, Conhost\Server:Clean, Conhost\Server:Rebuild, Conhost\Server:Publish]
$matchingTargets = $targets | Where-Object { $_.MSBuild.Condition -eq $msBuildCondition }

# If we didn't find a target, it's possible that the project didn't have a
# .metaproj in OpenConsole.sln.metaproj. Try filtering again, but leave off the
# .metaproj extension.
if ($matchingTargets.length -eq 0)