biggy/serumalt
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
4748bfe768c746080d6bf4d1206caa3d8cbd5af7
serumalt/AUDIT_REPORT.md
T
biggy 05fd6440bd docs: add agent guidance and historical audit report 
Add root AGENT.md with project conventions, build verification steps,
source layout, style rules, and real-time/concurrency requirements.
Add per-module AGENT.md files for each existing and proposed source
subdirectory. Add AUDIT_REPORT.md as a historical Phase 1 snapshot
documenting memory management, error handling, concurrency model,
naming conventions, and anti-pattern catalog.
2026-09-09 13:25:02 +02:00

29 KiB
Raw Blame History

SerumAlt codebase audit (Phase 1)

Historical snapshot, taken before the platform/harness build separation. Build commands, defaults, target locations and line numbers below describe that earlier state. Use README.md's Building section and the current CMake files for builds. The proposed source moves are suggestions, not an approved implementation plan.

Scope: the SerumAlt C++/JUCE VST3 + Standalone synthesiser. This report records the implicit standards, anti-patterns, memory choices, error handling, concurrency model and module boundaries found by reading the real repository files. No source, build or documentation files were modified. All findings are cited with path:line references read during this task.

Repository note: at the start of this audit the working tree already had uncommitted changes to CMakeLists.txt, several Source/ files and several third_party/JUCE/ files, plus untracked build/, build_windows/ and SerumAlt_Windows/ directories. Those changes predate this audit and are not analysed here except where the current on-disk content is the source of truth.

1. Build & test setup

Project metadata and targets

  • Project name and version: project(SerumAlt VERSION 1.0.0 LANGUAGES CXX) at CMakeLists.txt:3.
  • C++ standard: C++17, required, extensions off at CMakeLists.txt:8-10 (CMAKE_CXX_STANDARD 17, CMAKE_CXX_STANDARD_REQUIRED ON, CMAKE_CXX_EXTENSIONS OFF).
  • The plugin target is SerumAlt, declared with juce_add_plugin(...) at CMakeLists.txt:33-46. Formats are VST3 AU Standalone at CMakeLists.txt:44. JUCE generates the concrete per-format targets (SerumAlt_VST3, SerumAlt_Standalone, and SerumAlt_AU on macOS). There is no separate PluginProcessor/PluginEditor target; those are ordinary classes compiled into SerumAlt.
  • A console test target SerumAltTest is added at CMakeLists.txt:126-140 via juce_add_console_app. It compiles the entire ${SERUMALT_SOURCES} list plus Source/Tests/TestMain.cpp (CMakeLists.txt:128).

Source list

Sources are an explicit list, not a glob. set(SERUMALT_SOURCES ...) spans CMakeLists.txt:50-87 and is consumed by target_sources(SerumAlt PRIVATE ${SERUMALT_SOURCES}) at CMakeLists.txt:89. The list enumerates every .cpp used by the plugin, including the subdirectory files Source/EffectUnits/*.cpp, Source/GUI/*.cpp and Source/Presets/FactoryPresets.cpp.

Two header-only files are intentionally absent from the list because they have no translation unit: Source/EffectUnits/Biquad.h and Source/GUI/SerumLookAndFeel.h.

Consequence for later phases: any file move or rename must update this list, because there is no glob to pick files up automatically.

Custom flags and linkage

  • Compile definitions at CMakeLists.txt:91-95: JUCE_WEB_BROWSER=0, JUCE_USE_CURL=0, JUCE_VST3_CAN_REPLACE_VST2=0.
  • Link libraries at CMakeLists.txt:97-103: juce::juce_audio_utils and juce::juce_dsp (private), juce::juce_recommended_config_flags and juce::juce_recommended_warning_flags (public).
  • Warning flags at CMakeLists.txt:142-146: /W4 under MSVC, -Wall -Wextra otherwise.
  • MinGW cross-build static runtime at CMakeLists.txt:113-118: when CMAKE_CROSSCOMPILING, SerumAlt_VST3 gets -static-libgcc -static-libstdc++ -static so the DLL is self-contained.
  • VST3 manifest handling at CMakeLists.txt:23-31: SERUMALT_VST3_AUTO_MANIFEST is FALSE when cross-compiling (because JUCE 7.0.12 builds the manifest helper with the same toolchain, producing a Windows .exe that cannot run on the Linux host). The manifest is instead injected by build_windows.sh.

Windows cross-build (build_windows.sh)

build_windows.sh is the canonical Windows build path. Exact steps, in order:

  1. Verify x86_64-w64-mingw32-g++ exists, else exit with an error (build_windows.sh:16-21).
  2. Regenerate mingw64-toolchain.cmake from a heredoc inside the script (build_windows.sh:26-42). This is identical in content to the checked-in mingw64-toolchain.cmake (see below), so the toolchain file is defined in two places.
  3. Configure: cmake -S . -B build_windows -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=mingw64-toolchain.cmake -DSERUMALT_BUILD_TESTS=OFF (build_windows.sh:47-50). Note the test harness is explicitly disabled here.
  4. Build: cmake --build build_windows --target SerumAlt_VST3 (build_windows.sh:53).
  5. Inject Contents/Resources/moduleinfo.json into the .vst3 bundle by writing the file directly, because the automatic manifest step was disabled (build_windows.sh:63-126).
  6. Copy the bundle to SerumAlt_Windows/SerumAlt.vst3 (build_windows.sh:129-131).

The toolchain (mingw64-toolchain.cmake:1-15) sets CMAKE_SYSTEM_NAME Windows, CMAKE_SYSTEM_PROCESSOR x86_64, the x86_64-w64-mingw32-* compilers/tools, and CMAKE_FIND_ROOT_PATH /usr/x86_64-w64-mingw32 with PROGRAM NEVER and the other ..._MODE_* values ONLY.

Native Linux build (README only)

README.md:76-78 documents a native build:

cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release
cmake --build build

Outputs listed at README.md:80-84 are build/SerumAlt_artefacts/Release/VST3/SerumAlt.vst3 and the standalone app.

Tests

Tests exist. Source/Tests/TestMain.cpp is a console harness that instantiates the processor, plays notes through every factory preset, checks for NaN/inf and silence, and toggles RAVE (TestMain.cpp:37-135). It returns a non-zero exit code on failure (TestMain.cpp:134).

Tests are run through the native build, documented at README.md:96-98:

cmake --build build --target SerumAltTest
./build/SerumAltTest_artefacts/Release/SerumAltTest

The Windows cross-build does not build or run tests: build_windows.sh:50 passes -DSERUMALT_BUILD_TESTS=OFF.

Clean build and test command

There is no clean step in build_windows.sh. The script configures into build_windows/ and builds without removing or cleaning it first. A clean rebuild is not scripted; it would require manually removing build_windows/ (or invoking Ninja's clean target, which the script does not do). A combined "clean build + run tests" command does not exist: the Windows script disables tests, and the README test instructions assume an already-built native build/ tree.

2. Proposed module boundaries

The codebase already has three real subdirectory boundaries (EffectUnits/, GUI/, Presets/) plus Resources/ (assets only) and Tests/. The flat files at Source/ root mix four distinct concerns, and three root files clearly belong inside existing subdirectories. The proposal below is based on the include graph actually present, not on generic synth architecture.

Dependency facts that drive the split

  • Params.h/Params.cpp depend only on <JuceHeader.h> (Params.h:3). It declares the ids, maps namespaces, all enums, ModSource/ModTarget, and the wavetable name table. It is the shared foundation: nearly every other file includes it.
  • Wavetable, Oscillator, SubOscillator, NoiseOscillator, Envelope, LFO, Filter, FilterBank, SynthVoice depend only on Params, JuceHeader, and each other (Oscillator.h:4-5, FilterBank.h:4-5, SynthVoice.h:4-12).
  • ModulationMatrix and MacroControls depend only on Params (ModulationMatrix.h:4, MacroControls.h:4).
  • RAVEButton.h/RaveController depends only on Params (RAVEButton.h:4) but manipulates the APVTS, so it sits between modulation and plugin plumbing.
  • FXProcessor.h depends only on Params (FXProcessor.h:4); every file in EffectUnits/ includes ../FXProcessor.h (Hyper.h:3, Chorus.h:3, etc.), so FXProcessor is the base the effect units build on.
  • Engine aggregates the DSP: Engine.h:4-10 includes Wavetable, SynthVoice, LFO, FXProcessor, ModulationMatrix, MacroControls.
  • PluginProcessor.h includes Params, Engine, RAVEButton (PluginProcessor.h:4-6); it is the JUCE AudioProcessor boundary plus state/preset/RAVE glue.
  • PluginEditor.h includes PluginProcessor and every GUI/ header (PluginEditor.h:4-14); it is pure GUI.
  • Resources.h depends on JuceHeader plus Params for the format helpers (Resources.cpp:2); it holds the theme and SVG strings. The SVG asset files live in Source/Resources/, but the Resources.{h,cpp} code lives at root, splitting one concept across two locations.
  • Presets/FactoryPresets depends on Params and ModulationMatrix (FactoryPresets.h:4-5).
  • Tests/TestMain.cpp depends only on PluginProcessor (TestMain.cpp:4).

Proposed modules

Module Files (current location) Depends on
params (foundation) Params.{h,cpp} JuceHeader only
synth (DSP building blocks + voice) Wavetable.{h,cpp}, Oscillator.{h,cpp}, SubOscillator.{h,cpp}, NoiseOscillator.{h,cpp}, Envelope.{h,cpp}, LFO.{h,cpp}, Filter.{h,cpp}, FilterBank.{h,cpp}, SynthVoice.{h,cpp} params
modulation ModulationMatrix.{h,cpp}, MacroControls.{h,cpp}, RAVEButton.{h,cpp} params
effects FXProcessor.{h,cpp} plus all of EffectUnits/ (including Biquad.h) params
engine Engine.{h,cpp} synth, effects, modulation
gui PluginEditor.{h,cpp} plus all of GUI/ (including SerumLookAndFeel.h) params, synth (for wavetable display), plugin
resources Resources.{h,cpp} plus Resources/*.svg params
presets Presets/FactoryPresets.{h,cpp} params, modulation
plugin (JUCE plumbing) PluginProcessor.{h,cpp} engine, modulation, presets, params
tests Tests/TestMain.cpp plugin

Where the current layout violates the boundaries

  • FXProcessor.{h,cpp} sits at Source/ root but is the base class and owner of the EffectUnits/ classes; EffectUnits/*.h include it as ../FXProcessor.h. It belongs in EffectUnits/ (or in a renamed FX/ directory with the units).
  • PluginEditor.{h,cpp} sits at root but is pure GUI and includes every GUI/ header. It belongs in GUI/.
  • Resources.{h,cpp} holds the theme and SVG strings, while the .svg assets live in Source/Resources/. The code and assets should live together under one resources module.
  • RAVEButton.{h,cpp} is orphaned at root under a filename that does not match its class (RaveController). It is a macro-like boost over the APVTS, so it fits the modulation module, though its APVTS manipulation also makes plugin a defensible home.
  • The remaining root files (Params, Wavetable, Oscillator, SubOscillator, NoiseOscillator, Envelope, LFO, Filter, FilterBank, SynthVoice, ModulationMatrix, MacroControls, Engine, PluginProcessor) are the synthesis core and JUCE boundary. Grouping them into synth/ and modulation/ subdirectories would match the dependencies above, but the flat layout is at least internally consistent: they are the only files that legitimately belong at root today.

3. Memory management standards

Dominant idiom: value semantics plus std::unique_ptr for ownership

  • Owned sub-objects are held by value, not by pointer: juce::AudioProcessorValueTreeState parameters; Engine engine; RaveController rave; at PluginProcessor.h:56-58; the Engine holds std::array<SynthVoice, kNumVoices> voices, std::array<LFO, kNumLfos> lfos, and WavetableLibrary, FXProcessor, ModulationMatrix, MacroControls by value at Engine.h:45-50.
  • Owned polymorphic objects use std::unique_ptr: std::array<std::unique_ptr<FXUnit>, 9> units at FXProcessor.h:50, populated with std::make_unique at FXProcessor.cpp:15-26.
  • GUI attachments are owned by std::unique_ptr: std::vector<std::unique_ptr<... SliderAttachment>> and ...ComboBoxAttachment at PluginEditor.h:103-104, filled at PluginEditor.cpp:169-170 and 183-184.
  • Transient GUI objects are std::unique_ptr: std::unique_ptr<juce::Label> valuePopup at Knob.h:38, created at Knob.cpp:61 and released with valuePopup.reset() at Knob.cpp:83.
  • JUCE-managed objects are returned as raw new pointers because JUCE takes ownership through addAndMakeVisible or the plugin API: createEditor() returns new PluginEditor (*this) at PluginProcessor.cpp:195; the plugin entry point returns new serum::SerumAltAudioProcessor() at PluginProcessor.cpp:380; GUI children are created with new at PluginEditor.cpp:166,178,193,602,606. These raw pointers are never deleted by the code, which is correct only because JUCE's component tree and plugin host own them.
  • Non-owning cross-object references are raw pointers or references: juce::RangedAudioParameter* param at ToggleButton.h:38, const WavetableLibrary* wtLib at WaveformDisplay.h:24, and the RenderContext raw const pointers to the library/matrix/macros at SynthVoice.h:24,32-33.

Containers and buffers

  • std::vector<float> is the standard delay line / table buffer: Wavetable.h:24 (frames), Filter.h:39 (combLine), Chorus.h:21, Delay.h:21, Reverb.h:22-29.
  • std::array is used for fixed-size DSP state: Oscillator.h:62 (std::array<SubVoice, kMaxUnison>), Filter.h:36,44, Reverb.h:22-27.
  • juce::AudioBuffer<float> is used for block buffers: Engine.h:59 (mixBuffer), SynthVoice.h:95 (scratch), FXProcessor.h:51 (dry, wet). These are sized once in prepare() (for example Engine.cpp:68, SynthVoice.cpp:23, FXProcessor.cpp:34-35) and cleared per block.

Realtime vs non-realtime allocation

The code intends to allocate only at prepare time, but it does not fully achieve this:

  • Prepare-time allocation is the norm: delay lines and buffers are sized in prepare() (Filter.cpp:24, Chorus.cpp:9-11, Reverb.cpp:17-33, Engine.cpp:68).
  • Violation 1: Engine::processBlock calls mixBuffer.setSize (2, n, false, false, true) on every block at Engine.cpp:211. AudioBuffer::setSize is a no-op when the size is unchanged, but it is still a per-callback reconfiguration call on the audio thread.
  • Violation 2: wavetables are built lazily. WavetableLibrary::getTable builds the table on first access (Wavetable.cpp:126-132) and getTable is reached from the audio path at SynthVoice.cpp:177-178. The prebuild() method exists at Wavetable.cpp:120-124 but is never called (it is not referenced anywhere else, and Engine::prepare at Engine.cpp:58-70 does not call it). The first rendered block for a wavetable therefore allocates 256 x 2048 floats plus harmonic work buffers on the audio thread.

4. Error handling

There is no exception handling and no user-facing alerting. Failures are handled with a mix of asserts, null guards, and return codes.

  • jassert is used exactly once in the entire codebase: jassert (freqHz > 0.0) at Oscillator.cpp:20. There are no throw, try or catch statements (verified by grep).
  • Null guards with silent fallback are the dominant pattern. The APVTS helper v() returns 0.0f when a parameter is missing: Engine.cpp:8-13. State restore silently returns on malformed input: PluginProcessor.cpp:313-315 returns when the XML is null and PluginProcessor.cpp:318-319 returns when the ValueTree is invalid. Preset loads skip missing parameters with if (auto* param = ...) at PluginProcessor.cpp:248-250.
  • Return codes carry failure for the two bounded collections: ModulationMatrix::addConnection returns false when the target is None or the connection limit is reached (ModulationMatrix.cpp:6-12), and MacroControls::addAssignment returns false similarly (MacroControls.cpp:6-13). These return values are ignored by their callers (PluginEditor.cpp:536, PluginEditor.cpp:687, PluginProcessor.cpp:254, PluginProcessor.cpp:258).
  • Out-of-range lookups degrade to empty/default values: getProgramName returns an empty string for a bad index (PluginProcessor.cpp:218-224); the enum-string converters fall through to return {} (Params.cpp:27,44,91).
  • There are no juce::AlertWindow, no MessageManager calls, and no logging in the plugin itself. The only reporting is the test harness printing to std::cout and returning an exit code (TestMain.cpp:73,103,129,132-134).

Net style: programmer errors use jassert (very sparingly), runtime/state errors use null guards or default values, and bounded-insertion failures use bool return codes that callers currently ignore.

5. Concurrency

The codebase is single-threaded and has no explicit synchronisation. A grep for threads, mutexes, locks, atomics, CriticalSection, SpinLock and ScopedLock returns nothing outside JUCE's own headers.

  • The only non-audio execution context is the JUCE message thread, driven by a juce::Timer: PluginEditor inherits juce::Timer (PluginEditor.h:24), starts it at 30 Hz (PluginEditor.cpp:135), and refreshes visuals in timerCallback (PluginEditor.cpp:875-880).
  • The audio thread is SerumAltAudioProcessor::processBlock (PluginProcessor.cpp:187-191) delegating to Engine::processBlock (Engine.cpp:176-318). It reads the playhead tempo at Engine.cpp:184-187.

The significant issue is unsynchronised sharing between the message thread and the audio thread:

  • The GUI mutates ModulationMatrix::connections via addConnection/removeConnection/ clear (PluginEditor.cpp:536,549,559) while the audio thread iterates the same vector in SynthVoice::render at SynthVoice.cpp:114. connections is a public std::vector (ModulationMatrix.h:30).
  • The GUI mutates MacroControls::assignments (PluginEditor.cpp:687,699) while the audio thread iterates it at SynthVoice.cpp:125. assignments is a public std::array of vectors (MacroControls.h:28).
  • The GUI writes LFO shape data through engine.setLfoShapeData (PluginEditor.cpp:483-486) which calls LFO::setShapeData (Engine.cpp:84-88, LFO.cpp:51-59), while the audio thread reads the same shapeBuffer in LFO::shapeValue (LFO.cpp:82-91).

None of these shared collections are locked or double-buffered. This is a real data race today: resizing connections while the audio thread iterates it is undefined behaviour.

The processor header acknowledges GUI-thread access but does not address the race: the comment "Public DSP state (read/write from the GUI thread)" at PluginProcessor.h:55 only describes intent.

6. Naming & style conventions

  • Namespace: every file is inside namespace serum (PluginProcessor.h:8, Engine.h:12, etc.). File-local helpers use an anonymous namespace (Engine.cpp:6, SynthVoice.cpp:6, PluginEditor.cpp:6, FactoryPresets.cpp:6).
  • Include guard: #pragma once in every header (for example PluginProcessor.h:1, Params.h:1, Biquad.h:1). No #ifndef guards exist.
  • Include order: <JuceHeader.h> first, then local headers (PluginProcessor.h:3-6). Subdirectory files include root files with a ../ relative path (Hyper.h:3, Knob.h:5, FactoryPresets.h:4).
  • File/header pairs: one primary class per .h/.cpp pair, with the pair named after the class (Oscillator.h/Oscillator.cpp, FilterBank.h/FilterBank.cpp). Header-only helpers exist where they have no state logic worth a TU (Biquad.h, SerumLookAndFeel.h).
  • Class and struct names: PascalCase, no prefix (SynthVoice, RenderContext, FilterBankParams, ModConnection).
  • Members: no m_ prefix and no trailing underscore. Plain camelCase fields such as note, velocity, baseFreq, active, released, noteId at SynthVoice.h:83-88, and sr, blockSize, bpm, pitchBend at Engine.h:52-57.
  • Constants: k prefix for compile-time constants (kNumVoices, kNumLfos at Params.h:221-227; kFrames, kTableSize at Wavetable.h:18-19; kMaxConnections at ModulationMatrix.h:28; kMaxAssignments at MacroControls.h:26). Local constant arrays in anonymous namespaces also use k (kEnvAttack at Engine.cpp:30, kFxType at PluginEditor.cpp:97).
  • Enums: enum class (scoped) with PascalCase enumerators (WarpMode::BendPlus, ModSource::Lfo1, ModTarget::Filter1Cutoff) at Params.h:213-219 and Params.h:232-256.
  • Namespaces for related constants: ids:: for parameter ID strings (Params.h:18), theme:: for colours (Resources.h:12), maps:: for unit mappings (Params.h:309).
  • Method naming: camelCase with a leading verb (noteOn, processAdd, getValue). JUCE overrides keep JUCE's spelling (processBlock, prepareToPlay, resized, timerCallback).
  • noexcept is applied consistently to the per-sample DSP hot paths (Oscillator.cpp:93, Filter.cpp:181,236, Wavetable.cpp:81, Envelope.cpp:54, SynthVoice.cpp:72, LFO.cpp:98) and to cheap getters (SynthVoice.h:71-74).
  • Formatting: two-space indentation; a space before the opening parenthesis in function calls and definitions (f (x) at PluginProcessor.cpp:12, juce::jlimit (...); if (...), for (...)). Aligned member initialiser lists (PluginEditor.cpp:113-115).
  • Section dividers: // ===... banners at class/file tops and // ---... dividers within files (PluginProcessor.cpp:17,198,261, PluginEditor.cpp:161).
  • Casting idiom: (size_t) casts on int loop indices when indexing std::vector/std::array (Engine.cpp:220, SynthVoice.cpp:79, FactoryPresets.cpp throughout), and (int) casts on size() results.

7. Anti-pattern catalog

Each entry lists the location, the problem, and the standard that a later AGENT.md phase should encode.

  1. Parameter ID tables duplicated three times.

    • Params.h:18-208 (canonical ids::), Engine.cpp:30-55 (kEnv*, kLfo*, kFx*), PluginEditor.cpp:97-108,433-437,458-464,820-825,833 (the same arrays again), plus the display names and defaults in PluginProcessor.cpp:31-171.
    • Standard: one source of truth in Params.h (or a single generated table); engine and editor must reference it rather than re-declaring ID arrays.

  2. Wavetable library allocates on the audio thread.

    • WavetableLibrary::getTable builds lazily (Wavetable.cpp:126-132), reached from SynthVoice::render (SynthVoice.cpp:177-178); prebuild() is defined (Wavetable.cpp:120-124) but never called.
    • Standard: call prebuild() (or build all tables) in prepareToPlay, never in the render path.

  3. Per-block buffer resize in the audio callback.

    • Engine::processBlock calls mixBuffer.setSize(...) every block at Engine.cpp:211.
    • Standard: size the buffer once in prepare() and only clear() per block.

  4. Unsynchronised GUI/audio sharing of container state.

    • ModulationMatrix::connections is public (ModulationMatrix.h:30), written by the GUI (PluginEditor.cpp:536,549,559) and read on the audio thread (SynthVoice.cpp:114). MacroControls::assignments is public (MacroControls.h:28), written (PluginEditor.cpp:687,699) and read (SynthVoice.cpp:125). LFO shapeBuffer is written via setShapeData (LFO.cpp:51-59) and read (LFO.cpp:82-91).
    • Standard: either protect these collections with a lock, or use atomic/double-buffered exchange between the message and audio threads.

  5. Class name does not match its file.

    • Source/RAVEButton.h declares RaveController (RAVEButton.h:14), and RAVEButton.cpp implements it.
    • Standard: file pair name matches the class (RaveController.h/.cpp), or the class is renamed to match the file.

  6. Dead code in SubOscillator::noteOn.

    • SubOscillator.cpp:9-12 computes mult and then discards it with (void) freqHz; (void) mult; because the octave shift is already baked into the frequency by the voice.
    • Standard: remove the unused parameters or make the signature reflect what is actually used (the octave shift is applied at SynthVoice.cpp:179).

  7. No-op override.

    • changeProgramName is an empty override at PluginProcessor.cpp:226-228.
    • Standard: implement it or do not override it.

  8. Ignored prepare-time parameter.

    • Filter::prepare accepts maxBlockSize and discards it with (void) maxBlockSize; (Filter.cpp:21-29); effect units name the parameter int but never use it (Hyper.cpp:6, Chorus.cpp:6, Flanger.cpp:6, etc.).
    • Standard: keep the FXUnit interface uniform but do not add unused parameters to concrete prepare() implementations.

  9. Duplicated envelope curve constants.

    • The attack/decay shape mapping 0.3 + curve * 2.7 / 3.0 - curve * 2.7 appears in Envelope.cpp:44-45 and again in the display EnvelopeDisplay.cpp:19-20.
    • Standard: expose one shared helper so the preview cannot drift from the DSP.

  10. Filter response reimplemented for display.

    • FilterDisplay::magnitude (FilterDisplay.cpp:6-36) hardcodes a parallel set of transfer-function approximations that duplicate the real DSP in Filter.cpp (Filter.cpp:87-234).
    • Standard: keep the display response derived from one documented source, or mark it as an approximation with a comment that the two are not the same code.

  11. Inconsistent pi constants.

    • kTwoPi is defined at Oscillator.h:67 and SubOscillator.h:26; twoPi at Wavetable.cpp:21; kPi at Wavetable.cpp:156 and Filter.cpp:8; raw literals 6.28318530717958647692 appear at Chorus.cpp:40-41, Flanger.cpp:38-39, Phaser.cpp:30-31, and 6.2831853f at LFO.cpp:67 and LFODisplay.cpp:19.
    • Standard: use juce::MathConstants<T>::pi / twoPi everywhere.

  12. Public mutable state exposes internals.

    • PluginProcessor.h:56-58 exposes parameters, engine and rave as public fields; ModulationMatrix.h:30 and MacroControls.h:28 expose their collections; Engine.h:36-38 returns non-const references from getMatrix(), getMacros() and getWavetables().
    • Standard: return const& from accessors and route mutation through member functions, except where a public field is an explicit, documented design choice.

  13. Unchecked raw parameter dereference.

    • PluginEditor.cpp:794 dereferences getRawParameterValue(id)->load() without a null check, while the equivalent helper in Engine.cpp:8-13 guards the pointer.
    • Standard: always guard getRawParameterValue/getParameter results before use.

  14. Mixed ownership for GUI children.

    • makeKnob/makeCombo/makeToggle return raw pointers to new-allocated children that JUCE owns via addAndMakeVisible (PluginEditor.cpp:162-199), and fxUp/ fxDown use raw new at PluginEditor.cpp:602,606. This is correct only because of JUCE's component-tree ownership, but it is easy to misread as a leak.
    • Standard: document that JUCE-owned children are raw pointers and never deleted, and keep std::unique_ptr for everything the plugin owns directly.

8. Hard constraints worth codifying

Only rules that the current code and build actually support are listed.

  1. Do not allocate or resize containers in the audio callback. Evidence that this is the intent: all DSP state is sized in prepare() (Engine.cpp:68, SynthVoice.cpp:23, FXProcessor.cpp:34-35) and the code uses pre-sized juce::AudioBuffer and std::array. The current violations (Engine.cpp:211 and the lazy wavetable build) are exceptions to fix, not the rule.
  2. All audio parameters are normalised to 0..1 in the APVTS, and physical units are mapped only through maps:: (Params.h:309-342). DSP must consume the normalised values, not physical units.
  3. Parameter IDs live in the ids namespace in Params.h (Params.h:18-208). Engine, editor and preset code must reference ids::, not re-declare string literals. (This is the stated intent at Params.h:5-9; the current duplication in Engine.cpp and PluginEditor.cpp is a violation to eliminate.)
  4. The CMake source list is explicit (CMakeLists.txt:50-87), not a glob. Every file move or rename must update SERUMALT_SOURCES.
  5. All code is inside namespace serum, and every header uses #pragma once (verified across all headers).
  6. No exceptions and no user-facing alert dialogs. Use jassert for programmer errors, null guards for runtime lookups, and bool returns for bounded inserts (see Oscillator.cpp:20, Engine.cpp:8-13, ModulationMatrix.cpp:6-12).
  7. When cross-compiling for Windows, keep the VST3 manifest step disabled and let build_windows.sh inject moduleinfo.json, because JUCE's manifest helper must not be built as a Windows executable on a Linux host (CMakeLists.txt:23-31, build_windows.sh:55-126). Keep the injected metadata in sync with PLUGIN_CODE/PLUGIN_MANUFACTURER_CODE/VERSION (build_windows.sh:60-62).
  8. The project is single-threaded by design. Any future thread, mutex or atomic must come with an explicit plan for the GUI-to-audio boundary, because the current matrix/macro/LFO state is shared without synchronisation (section 5).
Reference in New Issue View Git Blame Copy Permalink