Skip to content

Commit 7d9bba2

Browse files
committed
Fix remaining issues from the v2 review
- pluginval test: --record-missing=<value> is an error rather than the value being taken as the config path. - Message-thread helpers share one callOnMessageThreadAndWait() that rethrows an exception on the calling thread, so a throw can't leave the caller waiting forever. Used by the VST3 helpers, the editor helpers, the plugin scan, message-thread tests and the acceptance plugin teardown, which now can't throw out of its destructor. - Child process output is decoded with ChildProcessOutputDecoder, so a UTF-8 character split across two reads is no longer garbled. - operator delete no longer throws (it's noexcept, so that terminated the process); operator new throws std::bad_alloc rather than returning null. Removed the unused violationError(). - Replaced the em/en dashes in CLAUDE.md and the acceptance design doc.
1 parent 308a982 commit 7d9bba2

10 files changed

Lines changed: 239 additions & 175 deletions

File tree

‎CLAUDE.md‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -217,11 +217,11 @@ The command line is structured into subcommands, peeled off by a thin verb
217217
dispatcher in front of the settings pipeline (it does **not** use CLI11-native
218218
subcommands, so the env/config/CLI layering below is untouched):
219219

220-
- `pluginval validate [options] <plugin>` — the default; `<plugin>` is a
220+
- `pluginval validate [options] <plugin>` - the default; `<plugin>` is a
221221
positional argument. `pluginval <plugin>` and `pluginval [options] <plugin>`
222222
(no verb) also resolve to validate.
223-
- `pluginval run-tests` — runs the internal unit tests.
224-
- `pluginval strictness-help [level]` — lists tests at a strictness level.
223+
- `pluginval run-tests` - runs the internal unit tests.
224+
- `pluginval strictness-help [level]` - lists tests at a strictness level.
225225

226226
`settings_parser::dispatch()` (in `SettingsParser.cpp`) takes the `tokenise()`d
227227
command line and returns a `DispatchResult { command, validateTokens,
@@ -248,7 +248,7 @@ that CLI11 binds to directly. A single instance is filled by successive layers,
248248
**lowest to highest precedence: defaults → environment → `--config` → CLI**
249249
(in `SettingsParser::parseTokens`):
250250

251-
1. **preprocess** the raw command line — rewrite the deprecated `strictnessLevel`,
251+
1. **preprocess** the raw command line - rewrite the deprecated `strictnessLevel`,
252252
strip the macOS `-NSDocumentRevisionsDebugMode YES` flag, and insert an
253253
implicit `--validate` when the last argument is a bare plugin path.
254254
2. **Environment layer.** Env-var names are *derived* from the registered
@@ -267,7 +267,7 @@ uses a `CheckedTransformer`, and the hex/int seed is a small callback.
267267
`PluginTests::Options` at the boundary.
268268

269269
Adding a new option is three edits: a struct member, an entry in the nlohmann
270-
macro list, and one `add_option(...)` line — its environment variable then works
270+
macro list, and one `add_option(...)` line - its environment variable then works
271271
automatically. `SettingsSerializer` handles JSON load/save plus the two
272272
remaining conversions (hex seed, disabled-tests file).
273273

@@ -279,7 +279,7 @@ are handled by CLI11 (auto usage + a footer with the env-var/commands notes).
279279
### Acceptance Testing (`pluginval test`)
280280

281281
A **parallel subsystem** to validate, in `source/acceptance/`. It answers "does
282-
this plugin produce the expected output for a known input + state?" — a
282+
this plugin produce the expected output for a known input + state?" - a
283283
deterministic *render + golden-file comparison*, not a unit-test pass/fail. Full
284284
spec: `tests/acceptance/Acceptance testing design.md`; end-user guide:
285285
`docs/Acceptance testing.md`.
@@ -295,7 +295,7 @@ spec: `tests/acceptance/Acceptance testing design.md`; end-user guide:
295295
`acceptance::TestRunner`, which renders on a **background thread** (message
296296
thread left free; plugin deleted on it), with a watchdog for the timeout, and
297297
quits via `callAsync` when done.
298-
- **Config**: `acceptance::TestConfig` (`TestConfig.cpp/h`) — std-typed struct,
298+
- **Config**: `acceptance::TestConfig` (`TestConfig.cpp/h`) - std-typed struct,
299299
**snake_case JSON keys** mapped via explicit `to_json`/`from_json` (members
300300
stay camelCase). It is **independent** of `PluginvalSettings` / the `--config`
301301
layering: the test config is a positional argument loaded standalone.
@@ -314,7 +314,7 @@ spec: `tests/acceptance/Acceptance testing design.md`; end-user guide:
314314
tolerance, default one 16-bit LSB = `1/32768`; `0` = bit-exact). Adding
315315
`spectrum`/`crosscorr`/etc. is one registry entry, no config/runner changes.
316316
- **Dogfood + self-tests**: three minimal deterministic `juce_add_plugin` targets
317-
behind `PLUGINVAL_BUILD_TEST_PLUGINS` — `tests/test_plugins/tone_generator/`
317+
behind `PLUGINVAL_BUILD_TEST_PLUGINS` - `tests/test_plugins/tone_generator/`
318318
(closed-form sine/square generator, phase resets on `prepareToPlay`),
319319
`tests/test_plugins/gain/` (a gain effect, dogfoods the `input.audio` path) and
320320
`tests/test_plugins/playhead_probe/` (writes the host transport to its output,
@@ -401,8 +401,8 @@ The VST3 validator (Steinberg's vstvalidator) is embedded into pluginval when bu
401401
5. When the `VST3validator` test runs, it spawns the extracted validator as a subprocess
402402
403403
**Key files:**
404-
- `cmake/GenerateBinaryHeader.cmake` — binary-to-C-header conversion script
405-
- `source/vst3validator/VST3ValidatorRunner.h/cpp` — extracts embedded binary, returns `juce::File`
404+
- `cmake/GenerateBinaryHeader.cmake` - binary-to-C-header conversion script
405+
- `source/vst3validator/VST3ValidatorRunner.h/cpp` - extracts embedded binary, returns `juce::File`
406406
407407
**Disabling embedded validator:**
408408
```bash
@@ -451,6 +451,8 @@ Located in `TestUtilities.h`:
451451
- `countNaNs()`, `countInfs()`, `countSubnormals()` - Audio validation
452452
- `ScopedEditorShower` - RAII editor creation/destruction
453453
- `callPrepareToPlayOnMessageThreadIfVST3()` - VST3-safe lifecycle
454+
- `callOnMessageThreadAndWait()` - Run code on the message thread and wait; rethrows its exceptions on the caller
455+
- `ChildProcessOutputDecoder` - Decode child-process output read in chunks without splitting UTF-8 characters
454456
- `ScopedAllocationDisabler` - Detect allocations in audio thread
455457

456458
### Real-time Safety Checking

‎source/CommandLineTests.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,11 @@ struct CommandLineTests : public juce::UnitTest
605605
const auto d = settings_parser::dispatch (settings_parser::tokenise ("test --output-dir=out config.json"));
606606
expect (d.error.contains ("--output-dir"));
607607
}
608+
{
609+
// A flag doesn't take a value, so the value isn't taken as the config path
610+
const auto d = settings_parser::dispatch (settings_parser::tokenise ("test --record-missing=1 config.json"));
611+
expect (d.error.contains ("--record-missing"));
612+
}
608613
}
609614

610615
beginTest ("Report writer: JUnit XML");

‎source/PluginTests.cpp‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -117,13 +117,10 @@ void PluginTests::runTest()
117117
{
118118
beginTest ("Scan for plugins located in: " + fileOrID);
119119

120-
juce::WaitableEvent completionEvent;
121-
juce::MessageManager::callAsync ([&, this]() mutable
122-
{
123-
knownPluginList.scanAndAddDragAndDroppedFiles (formatManager, juce::StringArray (fileOrID), typesFound);
124-
completionEvent.signal();
125-
});
126-
completionEvent.wait();
120+
callOnMessageThreadAndWait ([&]
121+
{
122+
knownPluginList.scanAndAddDragAndDroppedFiles (formatManager, juce::StringArray (fileOrID), typesFound);
123+
});
127124

128125
logMessage ("Num plugins found: " + juce::String (typesFound.size()));
129126
expect (! typesFound.isEmpty(),
@@ -212,13 +209,7 @@ void PluginTests::testType (const juce::PluginDescription& pd)
212209

213210
if (t->needsToRunOnMessageThread())
214211
{
215-
juce::WaitableEvent completionEvent;
216-
juce::MessageManager::callAsync ([&, this]() mutable
217-
{
218-
t->runTest (*this, *instance);
219-
completionEvent.signal();
220-
});
221-
completionEvent.wait();
212+
callOnMessageThreadAndWait ([&] { t->runTest (*this, *instance); });
222213
}
223214
else
224215
{

‎source/SettingsParser.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,12 @@ Precedence (lowest to highest): defaults, environment variables, --config, comma
400400
{
401401
const auto& token = tokensIn.getReference (i);
402402

403+
if (token.startsWith ("--record-missing="))
404+
{
405+
result.error = "--record-missing doesn't take a value (got \"" + token + "\")";
406+
return result;
407+
}
408+
403409
if (token.startsWith ("--") && token.contains ("="))
404410
{
405411
tokens.add (token.upToFirstOccurrenceOf ("=", false, false));

‎source/TestUtilities.cpp‎

Lines changed: 64 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,22 +28,6 @@ inline bool logAllocationViolationIfNotAllowed()
2828
return true;
2929
}
3030

31-
inline void violationError (const std::string& text)
32-
{
33-
switch (AllocatorInterceptor::getViolationBehaviour())
34-
{
35-
case AllocatorInterceptor::ViolationBehaviour::logToCerr:
36-
std::cerr << text;
37-
break;
38-
case AllocatorInterceptor::ViolationBehaviour::throwException:
39-
throw;
40-
break;
41-
case AllocatorInterceptor::ViolationBehaviour::none:
42-
default:
43-
break;
44-
}
45-
}
46-
4731
inline bool throwIfRequiredAndReturnShouldLog()
4832
{
4933
switch (AllocatorInterceptor::getViolationBehaviour())
@@ -60,6 +44,14 @@ inline bool throwIfRequiredAndReturnShouldLog()
6044
return false;
6145
}
6246

47+
/** The deallocation version: operator delete is noexcept, so a throw from it
48+
would terminate the process. Violations are still counted, just not thrown.
49+
*/
50+
inline bool shouldLogDeallocationViolation()
51+
{
52+
return AllocatorInterceptor::getViolationBehaviour() == AllocatorInterceptor::ViolationBehaviour::logToCerr;
53+
}
54+
6355
//==============================================================================
6456
#if JUCE_CLANG
6557
#define ATTRIBUTE_USED __attribute__((used))
@@ -73,7 +65,11 @@ ATTRIBUTE_USED void* operator new (std::size_t sz)
7365
if (throwIfRequiredAndReturnShouldLog())
7466
std::cerr << "!!! WARNING: Illegal allocation of " << sz << " bytes\n";
7567

76-
return std::malloc (sz);
68+
// operator new must never return null (malloc (0) may)
69+
if (auto ptr = std::malloc (sz > 0 ? sz : 1))
70+
return ptr;
71+
72+
throw std::bad_alloc();
7773
}
7874

7975
ATTRIBUTE_USED void* operator new[] (std::size_t sz)
@@ -82,13 +78,17 @@ ATTRIBUTE_USED void* operator new[] (std::size_t sz)
8278
if (throwIfRequiredAndReturnShouldLog())
8379
std::cerr << "!!! WARNING: Illegal array allocation of " << sz << " bytes\n";
8480

85-
return std::malloc (sz);
81+
// operator new must never return null (malloc (0) may)
82+
if (auto ptr = std::malloc (sz > 0 ? sz : 1))
83+
return ptr;
84+
85+
throw std::bad_alloc();
8686
}
8787

8888
ATTRIBUTE_USED void operator delete (void* ptr) noexcept
8989
{
9090
if (! logAllocationViolationIfNotAllowed())
91-
if (throwIfRequiredAndReturnShouldLog())
91+
if (shouldLogDeallocationViolation())
9292
std::cerr << "!!! WARNING: Illegal deletion\n";
9393

9494
std::free (ptr);
@@ -97,7 +97,7 @@ ATTRIBUTE_USED void operator delete (void* ptr) noexcept
9797
ATTRIBUTE_USED void operator delete[] (void* ptr) noexcept
9898
{
9999
if (! logAllocationViolationIfNotAllowed())
100-
if (throwIfRequiredAndReturnShouldLog())
100+
if (shouldLogDeallocationViolation())
101101
std::cerr << "!!! WARNING: Illegal array deletion\n";
102102

103103
std::free (ptr);
@@ -107,7 +107,7 @@ ATTRIBUTE_USED void operator delete[] (void* ptr) noexcept
107107
void operator delete (void* ptr, size_t) noexcept
108108
{
109109
if (! logAllocationViolationIfNotAllowed())
110-
if (throwIfRequiredAndReturnShouldLog())
110+
if (shouldLogDeallocationViolation())
111111
std::cerr << "!!! WARNING: Illegal deletion\n";
112112

113113
std::free (ptr);
@@ -116,7 +116,7 @@ void operator delete (void* ptr, size_t) noexcept
116116
void operator delete[] (void* ptr, size_t) noexcept
117117
{
118118
if (! logAllocationViolationIfNotAllowed())
119-
if (throwIfRequiredAndReturnShouldLog())
119+
if (shouldLogDeallocationViolation())
120120
std::cerr << "!!! WARNING: Illegal array deletion\n";
121121

122122
std::free (ptr);
@@ -253,3 +253,45 @@ struct AllocatorInterceptorTests : public juce::UnitTest,
253253
};
254254

255255
static AllocatorInterceptorTests allocatorInterceptorTests;
256+
257+
258+
//==============================================================================
259+
struct ChildProcessOutputDecoderTests : public juce::UnitTest
260+
{
261+
ChildProcessOutputDecoderTests()
262+
: juce::UnitTest ("ChildProcessOutputDecoder", "pluginval")
263+
{
264+
}
265+
266+
void runTest() override
267+
{
268+
beginTest ("Complete text passes straight through");
269+
{
270+
ChildProcessOutputDecoder decoder;
271+
expectEquals (juce::String (decoder.decode ("abc", 3)), juce::String ("abc"));
272+
}
273+
274+
beginTest ("A character split across reads is held back until complete");
275+
{
276+
const std::string text = "a\xe2\x82\xac" "b"; // "a€b", the euro sign is 3 bytes
277+
278+
for (size_t split = 1; split < text.size(); ++split)
279+
{
280+
ChildProcessOutputDecoder decoder;
281+
const auto first = decoder.decode (text.data(), split);
282+
const auto second = decoder.decode (text.data() + split, text.size() - split);
283+
284+
expect (first == text.substr (0, split < 4 ? 1 : split));
285+
expect (first + second == text);
286+
}
287+
}
288+
289+
beginTest ("Invalid bytes aren't held back");
290+
{
291+
ChildProcessOutputDecoder decoder;
292+
expect (decoder.decode ("a\xff", 2) == "a\xff");
293+
}
294+
}
295+
};
296+
297+
static ChildProcessOutputDecoderTests childProcessOutputDecoderTests;

0 commit comments

Comments
 (0)