Skip to content

Conversation

@DavidLiedle
Copy link

This commit addresses the long-standing FIXME comments in ProcessManagerImpl.cpp by implementing a custom error category for process exit codes. Previously, exit codes were incorrectly placed in the system_category, which would produce misleading error messages when calling .message() on the error code.

Changes:

  • Added process_exit_category_impl class that properly categorizes process exit codes
  • Implemented meaningful error messages for different exit scenarios:
    • "Process exited successfully" for exit code 0
    • "Process terminated by signal" for signal-terminated processes
    • "Process exited with code N" for non-zero exit codes
  • Updated both Windows and POSIX code paths to use the new category
  • Added comprehensive tests to verify the error messages and category

This improves debugging and error reporting when subprocess execution fails, making it easier for users and developers to understand what went wrong.

Fixes the FIXME comments at lines 101-107 and 112-116.

🤖 Generated with Claude Code

This commit addresses the long-standing FIXME comments in ProcessManagerImpl.cpp
by implementing a custom error category for process exit codes. Previously,
exit codes were incorrectly placed in the system_category, which would produce
misleading error messages when calling .message() on the error code.

Changes:
- Added process_exit_category_impl class that properly categorizes process exit codes
- Implemented meaningful error messages for different exit scenarios:
  - "Process exited successfully" for exit code 0
  - "Process terminated by signal" for signal-terminated processes
  - "Process exited with code N" for non-zero exit codes
- Updated both Windows and POSIX code paths to use the new category
- Added comprehensive tests to verify the error messages and category

This improves debugging and error reporting when subprocess execution fails,
making it easier for users and developers to understand what went wrong.

Fixes the FIXME comments at lines 101-107 and 112-116.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 9, 2025 20:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a custom error category for process exit codes to replace the incorrect use of system_category, providing meaningful error messages for different process termination scenarios.

  • Added a process_exit_category_impl class with appropriate error messages for successful exits, signal terminations, and exit codes
  • Updated both Windows and POSIX code paths to use the new error category instead of system_category
  • Added comprehensive tests to verify error messages and category names for different exit scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/process/ProcessManagerImpl.cpp Implements custom error category and updates exit code handling to use it
src/process/test/ProcessTests.cpp Adds tests for verifying error messages and category for different exit scenarios

Comment on lines +132 to +133
// Use sh -c "exit 42" to exit with specific code
auto evt = app->getProcessManager().runProcess("sh -c 'exit 42'", "").lock();
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using shell command with single quotes inside double quotes may cause issues on Windows. Consider using a more portable approach or adding platform-specific test sections.

Suggested change
// Use sh -c "exit 42" to exit with specific code
auto evt = app->getProcessManager().runProcess("sh -c 'exit 42'", "").lock();
// Use platform-specific command to exit with code 42
#ifdef _WIN32
std::string exitCmd = "cmd /C exit 42";
#else
std::string exitCmd = "sh -c \"exit 42\"";
#endif
auto evt = app->getProcessManager().runProcess(exitCmd, "").lock();

Copilot uses AI. Check for mistakes.
{
return "Process exited successfully";
}
else if (value == -1)
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number -1 for signal termination should be defined as a named constant to improve code readability and maintainability.

Suggested change
else if (value == -1)
else if (value == PROCESS_TERMINATED_BY_SIGNAL)

Copilot uses AI. Check for mistakes.
Comment on lines +139 to +140
// Use -1 to indicate signal termination in our custom category
return asio::error_code(-1, process_exit_category());
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number -1 for signal termination should be defined as a named constant to match the usage in the error message function and improve consistency.

Suggested change
// Use -1 to indicate signal termination in our custom category
return asio::error_code(-1, process_exit_category());
// Use PROCESS_TERMINATED_BY_SIGNAL to indicate signal termination in our custom category
return asio::error_code(PROCESS_TERMINATED_BY_SIGNAL, process_exit_category());

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant