feat: replace DocumentManager with RAII MusicXml - #435
Open
webern wants to merge 4 commits into
Open
Conversation
DocumentManager was a singleton that returned numeric document ids, held a map of shared_ptr<Document>, and required the client to call destroyDocument by hand. The registry, the ids, the badDocumentId error code, and the getUniqueId counter all existed only to support that scheme. They are gone. The public entry point is now a move-only mx::api::MusicXml that owns its core::Document and frees it at scope exit: auto doc = MusicXml::fromFile(path); // Result<MusicXml> auto score = intoScore(std::move(doc).value()); // consumes the doc auto doc = fromScore(score); // Result<MusicXml> doc.writeToFile(out); // Result<void> const auto& core = doc.getCoreDocument(); // escape hatch The free functions are the score conversions: getScore (const, non-consuming), intoScore (by value, consumes and frees the tree at return), and fromScore (authors a new document; can fail with the core writer's refusal errors). All four entry points preserve the Result error boundary: parse/serialize failures return ioError, xmlSyntaxError, the mirrored core parse codes, or internalError for any caught exception. A moved-from MusicXml fails safely on every path (internalError on read/write, an empty core document on getCoreDocument) rather than crashing. PartWriter's synthesized <score-instrument> ids used to come from DocumentManager::getUniqueId; they are now a process-wide atomic counter inside PartWriter, seeded at the same high value so they still avoid collisions with ids present in parsed documents. Tests, examples, and the README were ported from the id/destroyDocument dance to the RAII ownership model. The DocumentManager-based test file is replaced by MusicXmlTest, which also pins the moved-from behavior. The api/impl layers are the only ones touched; core, the generator, and the roundtrip baseline are unchanged. Closes #95.
webern
force-pushed
the
m/mxdev-raii
branch
from
September 12, 2026 14:54
73feb60 to
a48f262
Compare
webern
commented
Sep 12, 2026
Review changes to the MusicXml api: - The move operations now enforce the pimpl invariant: a moved-from MusicXml holds a valid, empty document (a default core::Document) rather than a null pimpl, so every operation on it is safe. The moved-from special cases in writeToFile/writeToStream/getScore are gone; the object guarantees the state. The move constructor allocates the empty replacement so it is not noexcept; move assignment swaps and is. - getCoreDocument is removed from the public api. clone() makes a deep copy instead. The core model is no longer reachable through the public surface at all; mx's own layers and tests reach it through the new private header mx/api/MusicXmlInternal.h (coreDocumentOf), a friend of MusicXml. - Header comments trimmed: the class comment is one sentence, the error variants stay in Result's own documentation, and fromScore keeps only its refusal semantics. - ResultCode::internalError carries a TODO about swallowing exception information. - withWriteVersion's comment reworded to say why it exists without describing the past. All 602 api test cases pass (5495 assertions), including a new clone test and a rewritten moved-from test pinning the empty-document behavior.
Reversing my previous take on the review: removing the escape hatch was a mistake. getCoreDocument is back on MusicXml, documented as an escape hatch for when mx::api does not do what you need and you want to edit the core DOM directly. The non-const overload allows editing; the const overload reads. The private header indirection (MusicXmlInternal.h and coreDocumentOf) is removed; the tests that assert on the core tree use the public method again.
The reference getCoreDocument returns is only good while this MusicXml is alive and has not been moved away. Moving into another MusicXml, or into intoScore (which frees the tree when it returns), leaves any reference taken beforehand dangling with no compiler diagnostic. Document this on the method.
webern
marked this pull request as ready for review
September 12, 2026 18:14
This was referenced Sep 12, 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.
Human Summary
This fixes a very old and very bad design, the
DocumentManageris no more. This should be a little more RAII friendly, though the footguns in C++ are pretty astounding after writing Rust for years.Summary
DocumentManagerwas a singleton returning numeric document ids, holding a map ofshared_ptr<Document>, and requiring the client to calldestroyDocumentby hand. The registry, the ids, thebadDocumentIderror code, and thegetUniqueIdcounter all existed only to support that scheme.They are gone. The public entry point is now a move-only
mx::api::MusicXmlthat owns itscore::Documentand frees it at scope exit:The free functions are the score conversions:
getScore(const, non-consuming),intoScore(by value, consumes and frees the tree at return), andfromScore(authors a new document; can fail with the core writer's refusal errors). All entry points preserve theResulterror boundary: parse/serialize failures returnioError,xmlSyntaxError, the mirrored core parse codes, orinternalErrorfor any caught exception. A moved-fromMusicXmlholds a valid, empty document rather than a null pimpl, so every operation on it stays safe.getCoreDocumentis an escape hatch for whenmx::apidoes not do what you need and you want to edit the core DOM directly; it requires including the privatemx::coreheaders and is not recommended (open an issue first).PartWriter's synthesized<score-instrument>ids used to come fromDocumentManager::getUniqueId; they are now a process-widestd::atomic<int>counter insidePartWriter, seeded at the same high value so they still avoid collisions with ids already present in parsed documents.Scope
src/include/mx/api/MusicXml.h,src/private/mx/api/MusicXml.cpp.src/include/mx/api/DocumentManager.h,src/private/mx/api/DocumentManager.cpp,src/private/mxtest/api/DocumentManagerTest.cpp.Result.h:badDocumentIdremoved; boundary comments updated toMusicXml.destroyDocumentdance to RAII ownership.DocumentManagerTestis replaced byMusicXmlTest, which also pins the moved-from behavior.mx::core, the generator, and the roundtrip baseline are untouched.This is a breaking change to
mx::api. Client code (komp,denigma) that calledDocumentManager::getInstance()must move toMusicXml/fromScore/getScore/intoScore.Testing
MX_RUNNING_IN_DOCKER=1 make api-buildbuilds mx + mxtest + the three examples + the api-roundtrip binary.MX_RUNNING_IN_DOCKER=1 make api-test: all tests passed (5495 assertions in 602 test cases). All three api examples ran successfully.make fmt-checkpasses.make fmt/fmt-checkcould not run locally (the lima docker VM mounts~read-only, so the container cannot runclang-format -iinto the source tree); the CI fmt-check will run.References
MusicXmlboundary preserves theResultquarantineDocumentManagerhad).