refactor: Stopped Configuration from leaking everywhere - #2307
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Spice86’s runtime wiring to reduce direct dependency on the CLI Configuration object by mapping it into subsystem-specific runtime option records (execution policy, GDB, DOS/program loading, memory dumps, and audio). This better isolates emulator components from CLI parsing concerns and centralizes option projection in one place.
Changes:
- Introduces
RuntimeOptionsMapperand a set of runtime option records (ExecutionPolicyOptions,GdbServerOptions,ProgramLoadOptions,DosOptions,MemoryDumpOptions,AudioRuntimeOptions) plus a sharedDosRuntimeState. - Refactors core components (execution policy, GDB server, program bootstrap/loaders, DOS, memory dump exporter, Sound Blaster/OPL) to consume runtime options instead of
Configuration. - Adds unit tests validating the configuration→options mapping.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Spice86.Tests/UI/BreakpointUiTestBase.cs | Updates UI test wiring to create MemoryDataExporter using MemoryDumpOptions/DosRuntimeState. |
| tests/Spice86.Tests/RuntimeOptionsMapperTests.cs | Adds unit tests covering RuntimeOptionsMapper projections and shared DosRuntimeState. |
| src/Spice86/Spice86DependencyInjection.cs | Creates runtime options via RuntimeOptionsMapper and injects them into emulator subsystems. |
| src/Spice86.Core/Emulator/StateSerialization/MemoryDataExporter.cs | Switches from Configuration to MemoryDumpOptions for callback replacement behavior. |
| src/Spice86.Core/Emulator/ProgramBootstrapper.cs | Switches from Configuration to ProgramLoadOptions, including shared DosRuntimeState inference. |
| src/Spice86.Core/Emulator/OperatingSystem/Structures/DosSysVars.cs | Accepts DosOptions instead of Configuration (XMS sizing decision). |
| src/Spice86.Core/Emulator/OperatingSystem/DosProgramLoader.cs | Accepts ProgramLoadOptions instead of Configuration (e.g., C: path). |
| src/Spice86.Core/Emulator/OperatingSystem/DosBatchProgramLoader.cs | Updates constructor to pass ProgramLoadOptions through to base loader. |
| src/Spice86.Core/Emulator/OperatingSystem/Dos.cs | Accepts DosOptions and routes relevant settings (drives, init, XMS/EMS) through options. |
| src/Spice86.Core/Emulator/Gdb/GdbServer.cs | Accepts GdbServerOptions instead of Configuration (port selection). |
| src/Spice86.Core/Emulator/ExecutionPolicy.cs | Accepts ExecutionPolicyOptions and routes GDB options through GdbServerOptions. |
| src/Spice86.Core/Emulator/Devices/Sound/OplConfig.cs | Removes old OPL config record (replaced by AudioRuntimeOptions). |
| src/Spice86.Core/Emulator/Devices/Sound/Opl3Fm.cs | Updates OPL synth construction to consume AudioRuntimeOptions. |
| src/Spice86.Core/Emulator/Devices/Sound/Blaster/SoundBlasterHardwareConfig.cs | Removes old SB hardware config record (replaced by AudioRuntimeOptions). |
| src/Spice86.Core/Emulator/Devices/Sound/Blaster/SoundBlaster.Definitions.cs | Updates internal config field type to AudioRuntimeOptions. |
| src/Spice86.Core/Emulator/Devices/Sound/Blaster/SoundBlaster.cs | Updates SB construction/config to use AudioRuntimeOptions (mixer flag, aliases). |
| src/Spice86.Core/CLI/RuntimeOptions/RuntimeOptionsMapper.cs | New mapper from CLI configuration to subsystem runtime option objects. |
| src/Spice86.Core/CLI/RuntimeOptions/ProgramLoadOptions.cs | New record: program bootstrap + DOS loader options. |
| src/Spice86.Core/CLI/RuntimeOptions/MemoryDumpOptions.cs | New record: memory dump exporter options. |
| src/Spice86.Core/CLI/RuntimeOptions/GdbServerOptions.cs | New record: GDB server endpoint/lifecycle options. |
| src/Spice86.Core/CLI/RuntimeOptions/ExecutionPolicyOptions.cs | New record: execution policy options (debug, stop-after-cycles, gdb). |
| src/Spice86.Core/CLI/RuntimeOptions/DosRuntimeState.cs | New shared mutable state for DOS init inference/consumption. |
| src/Spice86.Core/CLI/RuntimeOptions/DosOptions.cs | New record: DOS subsystem composition options. |
| src/Spice86.Core/CLI/RuntimeOptions/AudioRuntimeOptions.cs | New record: audio options (engine, MT-32 path, OPL, SB params) with compatibility aliases. |
Suppressed comments (1)
src/Spice86.Core/Emulator/OperatingSystem/Dos.cs:192
- The XML documentation for the
Dosconstructor contains a duplicated block of<param>entries (including a duplicateoptions), which will produce XML doc warnings/errors and makes the docs misleading. Remove the duplicated<param>lines and keep a single, complete param list.
/// <param name="options">DOS runtime options projected from the command-line configuration.</param>
/// <param name="memory">The emulator memory.</param>
/// <param name="functionHandlerProvider">Provides current call flow handler to peek call stack.</param>
/// <param name="stack">The CPU stack.</param>
/// <param name="state">The CPU state.</param>
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Added `ExecutionPolicyOptions` to encapsulate execution policy settings including debug mode and GDB server options. - Created `GdbServerOptions` to manage GDB server configuration, including the TCP port. - Implemented `MemoryDumpOptions` for memory dump exporters, utilizing shared DOS runtime state. - Developed `ProgramLoadOptions` for program bootstrap and DOS loaders, including executable path and arguments. - Introduced `RuntimeOptionsMapper` to convert command-line configurations into structured runtime options. - Refactored the Sound Blaster implementation to utilize new audio runtime options objects. - Updated various components of the emulator to use the new runtime options, ensuring consistency across the system. - Added unit tests for the `RuntimeOptionsMapper` to validate the mapping of configuration to runtime options.
maximilien-noal
force-pushed
the
refactor/configuration_leaks
branch
from
August 13, 2026 12:21
8da4a7c to
b628f7b
Compare
kevinferrare
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
ExecutionPolicyOptionsto encapsulate execution policy settings including debug mode and GDB server options.GdbServerOptionsto manage GDB server configuration, including the TCP port.MemoryDumpOptionsfor memory dump exporters, utilizing shared DOS runtime state.ProgramLoadOptionsfor program bootstrap and DOS loaders, including executable path and arguments.RuntimeOptionsMapperto convert command-line configurations into structured runtime options.RuntimeOptionsMapperto validate the mapping of configuration to runtime options.Rationale behind Changes
Stopped Configuration class from leaking into the entire code base. Lowers technical debt. Removes a "god" object, in other words.
Suggested Testing Steps
New tests were introduced. All should pass.