diff --git a/.github/release/Package.swift.template b/.github/release/Package.swift.template index 015d3d33d..7642079a4 100644 --- a/.github/release/Package.swift.template +++ b/.github/release/Package.swift.template @@ -1,7 +1,7 @@ // swift-tools-version: 5.9 // TEMPLATE for the machine-generated release manifest -- the Package.swift that -// exists only at a tagged release commit ("B" in docs/ai/design/release-process.md). +// exists only at a tagged release commit, called "commit B". // The _release-target-swift workflow interpolates the three MX_RELEASE // placeholders (version, url, checksum) and the release-build prep job commits // the result as B; commit R then restores the source manifest, so every tip of diff --git a/.github/workflows/_release-target-swift.yml b/.github/workflows/_release-target-swift.yml index 9bd8deb34..de8a08c02 100644 --- a/.github/workflows/_release-target-swift.yml +++ b/.github/workflows/_release-target-swift.yml @@ -1,4 +1,4 @@ -# The Swift release target (docs/ai/design/release-process.md §8-9): builds the +# The Swift release target builds the # Mx XCFramework from commit A and emits the two artifacts the release system # consumes -- the publishable archive and the metadata the prep job commits as B. # Called only from release-build.yml (workflow_call); never triggered directly. @@ -122,7 +122,7 @@ jobs: set -euxo pipefail # For a zip, swift package compute-checksum IS the file's SHA-256; # assert that equivalence here so release-publish can re-verify with - # plain sha256sum (design §5, check 6). + # plain sha256sum. CHECKSUM=$(swift package compute-checksum "out/dist/$ARCHIVE") SHA256=$(shasum -a 256 "out/dist/$ARCHIVE" | cut -d' ' -f1) if [ "$CHECKSUM" != "$SHA256" ]; then @@ -158,7 +158,7 @@ jobs: cp out/metadata/tree/Package.swift "$RUNNER_TEMP/manifest-check/" (cd "$RUNNER_TEMP/manifest-check" && swift package dump-package > /dev/null) - # The target's contract fragment (design §9): identity, artifact, + # The target's contract fragment: identity, artifact, # checksum, platforms, and the paths this target may occupy in B. A # copy ships in both artifacts. jq -n \ @@ -179,7 +179,7 @@ jobs: > out/metadata/fragment.json cp out/metadata/fragment.json out/dist/fragment.json - # The publishable artifact (design §6): archive + .sha256 + fragment. + # The publishable artifact: archive + .sha256 + fragment. # retention-days is the documented 90-day contract: the tag must be pushed # while this artifact still exists. - name: Upload release artifact @@ -190,7 +190,7 @@ jobs: retention-days: 90 if-no-files-found: error - # The prep-job channel (design §9, outputs b and c): the exact tree files + # The prep-job channel: the exact tree files # for commit B plus the contract fragment. - name: Upload metadata artifact uses: actions/upload-artifact@v4 diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 66d46266a..98bde14f1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -295,8 +295,7 @@ jobs: # Proves mx compiles as a Swift package under AppleClang/SPM on every PR -- # the path komp takes when it depends on a sibling mx via `.package(path:)`. - # Runs no test suites; the binary xcframework ships from release-build.yml - # (docs/ai/design/release-process.md), not from CI. + # Runs no test suites; the binary xcframework ships from release-build.yml, not from CI. # # SwiftPM has no compiler-launcher hook, so incremental builds come from caching # its .build tree: llbuild tracks inputs by content signature, so a restored diff --git a/.github/workflows/release-build.yml b/.github/workflows/release-build.yml index 0c8937bea..179f10d05 100644 --- a/.github/workflows/release-build.yml +++ b/.github/workflows/release-build.yml @@ -1,4 +1,4 @@ -# Phase 1 of the release system (docs/ai/design/release-process.md): build a +# Phase 1 of the release system: build a # chosen commit A for every release target, then open the release PR whose first # commit, B, is the tag target. Nothing here publishes; phase 2 is # release-publish.yml, triggered by the tag push after the PR merges. @@ -50,7 +50,7 @@ jobs: [[ "$VERSION" =~ $SEMVER ]] \ || { echo "::error::version must be MAJOR.MINOR.PATCH, optionally followed by a pre-release suffix such as -rc1, with no leading v, got '$VERSION'"; exit 1; } - # The target fan-out (design §9): one reusable workflow per release target. + # The target fan-out: one reusable workflow per release target. # Adding a target = add a job like this one, and extend the hardcoded path # ceiling in release-publish.yml with its declared metadata paths. swift: @@ -95,7 +95,7 @@ jobs: shopt -s nullglob if ! git merge-base --is-ancestor "$SHA" origin/main; then - echo "::warning::$SHA is not an ancestor of main yet; release-publish will refuse the tag until it is (design §5, check 3)." + echo "::warning::$SHA is not an ancestor of main yet; release-publish will refuse the tag until it is." fi FRAGMENTS=("$RUNNER_TEMP"/release-metadata/*/fragment.json) @@ -115,7 +115,7 @@ jobs: if [ -d "$d/tree" ]; then cp -R "$d/tree/." .; fi done - # release-manifest.json (design §5): version, base SHA A, this run's + # release-manifest.json: version, base SHA A, this run's # ID (the run pins itself into B -- dispatch inputs are not queryable # later), every target's fragment, and the union of paths B may touch. jq -s \ @@ -170,8 +170,7 @@ jobs: set -euo pipefail cat > "$RUNNER_TEMP/body.md" <<'BODY' Machine-generated release PR for **v__VERSION__**, base commit A = `__A__`, - opened by [release-build run __RUN_ID__](__RUN_URL__). Process: - `docs/ai/design/release-process.md`. + opened by [release-build run __RUN_ID__](__RUN_URL__). Two machine-written commits: diff --git a/.github/workflows/release-publish.yml b/.github/workflows/release-publish.yml index c8337d073..2c0c4993b 100644 --- a/.github/workflows/release-publish.yml +++ b/.github/workflows/release-publish.yml @@ -1,13 +1,13 @@ -# Phase 2 of the release system (docs/ai/design/release-process.md §5): a v* tag +# Phase 2 of the release system: a v* tag # push re-verifies commit B's provenance record against primary sources and # publishes the already-built artifacts. Publishing never rebuilds anything: the # bytes attached to the Release are the bytes release-build validated, fetched # from that run by the run ID pinned inside B. Any check failing means no -# release, loudly (design §11) -- recovery is always: delete the tag, fix the +# release, loudly -- recovery is always: delete the tag, fix the # cause, re-push. # # GitHub runs this file as it exists at the tagged commit, so only commits whose -# trees already carry release-publish.yml are releasable (design §4). +# trees already carry release-publish.yml are releasable. name: release-publish on: @@ -37,7 +37,7 @@ jobs: run: | set -euo pipefail fail() { - echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing (docs/ai/design/release-process.md, section 11)." + echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing." exit 1 } B=$(git rev-parse "refs/tags/$TAG^{commit}") @@ -109,7 +109,7 @@ jobs: run: | set -euo pipefail fail() { - echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing (docs/ai/design/release-process.md, section 11)." + echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing." exit 1 } # The API path scopes the lookup to this repository, so a run ID from @@ -127,7 +127,7 @@ jobs: run: | set -euo pipefail fail() { - echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing (docs/ai/design/release-process.md, section 11)." + echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing." exit 1 } : > assets.txt @@ -172,7 +172,7 @@ jobs: run: | set -euo pipefail fail() { - echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing (docs/ai/design/release-process.md, section 11)." + echo "::error::release-publish: $*. Nothing was published. Recover by deleting the tag (git push origin :refs/tags/$TAG), fixing the cause, and re-pushing." exit 1 } if gh release view "$TAG" --repo "$GITHUB_REPOSITORY" > /dev/null 2>&1; then diff --git a/AGENTS.md b/AGENTS.md index b390823b5..856297e1e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,46 +16,36 @@ You should, however, automatically fix stale code examples. ``` mx/ - AGENTS.md <- you are here - .claude/skills/ <- agent skills; every skill name starts with mx- - Makefile <- top-level build driver - cmake/ <- CMake helpers: GitSha.cmake, and the backend-pugixml consumer project - that `make backend-pugixml-test` builds (mx against a stock pugixml) - scripts/ <- maintenance scripts (vendor_pugixml.py refreshes src/private/mxpugi) - Dockerfile <- mx-sdk image with toolchains and dev tools - CMakeLists.txt <- C++ project - Package.swift <- Swift Package Manager wrapper (built by the swift CI job) - data/ <- MusicXML test corpus (large, see data/README.md) - docs/ai/design/ <- design docs - docs/musicxml-4.0-ed15c23.xsd <- the current musicxml.xsd schema - src/ <- C++ sourcecode root - src/include/mx/api/ <- the public mx::api headers; mx's public interface - src/private/ <- C++ source - mxpugi/ <- vendored pugixml 1.16 under the private mxpugi namespace (generated by - scripts/vendor_pugixml.py; XML parser and raw DOM) - mx/xml/ <- the XML seam: mx::xml::{Document,Node,Attribute} value handles, the - XmlBackend concept, and the pugixml adapter (mx/xml/pugixml/); the only - code under mx/ that includes mxpugi (docs/ai/design/xml-backend.md) - mx/core/ <- mx::core's hand-written runtime (outside of mx/core/generated) - mx/core/generated/ <- MusicXML XSD C++ typed model (generated, see gen/cpp) - mx/api/ <- mx::api implementation - mx/impl/ <- the api to core translation layer - mx/utility/ <- helpers - mx/examples/ <- api example programs - mxtest/core/ <- mx::core unit tests (small) - mxtest/corert/ <- C++ core roundtrip test (deserializes and serializes the test corpus) - mxtest/import/ <- normalization support (Normalize.cpp/h + DecimalFields.h) shared by corert and api tests - mxtest/api/ <- tests for the mx::api layer - mxtest/impl/ <- tests for the mx::impl layer - mxtest/file/ <- file-walking test infrastructure for the mx::api tests (MxFile, MxFileTest, etc.) - mxtest/control/ <- compile-control flags (CompileControl.h: enables/disables test suite compilation) - cpul/ <- vendored Catch2 test runner - gen/ <- code generator system (see gen/README.md) - cpp/ <- C++ target (the product): config.toml + templates - schema/ <- JSON Schema target - test/go/ <- A toy Go implementation of MusicXML XSD for gen validation - test/c/ <- A toy C implementation of MusicXML XSD for gen validation - audit/ <- MusicXML feature-audit tool (see audit/README.md); `make audit` + .claude/skills/ <- agent skills; every skill name starts with mx- + Makefile <- top-level build driver + cmake/ <- CMake helpers and the backend-pugixml consumer project + scripts/ <- maintenance scripts (vendor_pugixml.py refreshes mxpugi) + Dockerfile <- mx-sdk image with toolchains and dev tools + deslag.toml <- prose lint config; `make fmt-check` runs deslag + data/ <- MusicXML test corpus (large, see data/README.md) + docs/ai/design/ <- design docs + docs/musicxml-4.0-ed15c23.xsd <- the current musicxml.xsd schema + src/include/mx/api/ <- the public mx::api headers; mx's public interface + src/private/mxpugi/ <- vendored pugixml 1.16 in the mxpugi namespace (generated) + src/private/mx/xml/ <- the XML seam; the only mx/ code that includes mxpugi + src/private/mx/core/ <- mx::core's hand-written runtime + src/private/mx/core/generated/ <- MusicXML XSD C++ typed model (generated, see gen/cpp) + src/private/mx/api/ <- mx::api implementation + src/private/mx/impl/ <- the api to core translation layer + src/private/mx/utility/ <- helpers + src/private/mx/examples/ <- api example programs + src/private/mxtest/core/ <- mx::core unit tests (small) + src/private/mxtest/corert/ <- core roundtrip test over the test corpus + src/private/mxtest/import/ <- normalization shared by corert and api tests + src/private/mxtest/api/ <- tests for the mx::api layer + src/private/mxtest/impl/ <- tests for the mx::impl layer + src/private/mxtest/file/ <- file-walking infrastructure for the mx::api tests + src/private/cpul/ <- vendored Catch2 test runner + gen/ <- code generator system (see gen/README.md) + gen/cpp/ <- C++ target (the product): config.toml + templates + gen/schema/ <- JSON Schema target + gen/test/ <- toy Go and C targets that validate the generator + audit/ <- MusicXML feature-audit tool (see audit/README.md) ``` ## Feature audit (`audit/`) @@ -113,8 +103,8 @@ instead: - In code generator templates (`gen/cpp/templates/`): use `{{ident}}` to make names unique, e.g. `k{{ident}}Wire`, `isNameChar{{ident}}`. -Unity builds can be tested with CMake's built-in support — no changes to `CMakeLists.txt` are needed -by contributors. To verify, configure with: +Unity builds can be tested with CMake's built-in support; contributors do not need to change +`CMakeLists.txt`. To verify, configure with: ``` cmake -S . -B build/unity -DCMAKE_UNITY_BUILD=ON -DCMAKE_UNITY_BUILD_BATCH_SIZE=0 @@ -131,12 +121,12 @@ than the strongly-typed `mx::core` model. No core type may appear in a public ap - New fields that can be absent: use `std::optional` (values and sub-structs alike). Precedent: `NoteData::tieLetRing`, `PartData::transposition`, `MeasureData::partSymbol`. - Exception, enums: an absent-able enum gets an `unspecified` enumerator as its first value and - default — never `std::optional`. This includes the ternary + default, never `std::optional`. This includes the ternary `Bool { unspecified, yes, no }` in `ApiCommon.h`; use it, never `std::optional`. - Do not mass-migrate existing `bool is...Specified` / `has...` flags or `-1` sentinels; they are legacy (issue #249). Migrations are separate, deliberate breaking changes. - Every new field needs a default value and a matching `MXAPI_EQUALS_MEMBER(field)` line - (`MXAPI_DOUBLES_EQUALS_MEMBER` for doubles) in the type's equality block — a missed line silently + (`MXAPI_DOUBLES_EQUALS_MEMBER` for doubles) in the type's equality block. A missed line silently drops the field from equality and round-trip checks. ### Design doctrine @@ -147,21 +137,27 @@ no exceptions, `Result` quarantined), choice types, and comment rules. ## Quality gates -Run `make fmt` to format. `make fmt-check` is the clang-format gate **only** — it builds and tests -nothing. Run `make core-roundtrip-test` for corert (especially `mx/core` work) before pushing core -work. `make api-test` runs the api/impl suite + examples; `make api-roundtrip` and the gen/audit -gates run separately. `make test-all` runs every C++ suite at once (core roundtrip + unit + api-test -\+ api-roundtrip) — the deep gate CI runs on Linux (see `make help` / CI). Adding/removing a `data/` -file: bump the pinned count in `CoreRoundtripTest.cpp`, run `make audit` (regenerates `corpus.xml` + -`*.features.xml`), confirm round-trip via `make core-roundtrip-test`. `ApiLoadSmokeTest` proves a -file imports without crashing, not that the data is correct; the read→write→read gate -(`make api-roundtrip` / `roundtrip-baseline.txt`) is the correctness check — pin a fixture there to -defend a feature. `make wasm-test` builds `mx::api` for Emscripten and runs the examples under Node -— CI's check that mx still compiles and works client-side in a browser (a consumer, denigma, depends -on this; issue #386). `make backend-pugixml-test` builds mx against a stock pugixml -(`MX_XML_BACKEND=pugixml`, see the README's "Bringing your own XML library") and runs the core -suites — CI's check that the XML backend option works; `make symbol-check` only applies to the -default vendored build. +Run `make fmt` to format. `make fmt-check` is the formatting and prose gate: it runs clang-format +and `deslag check` (which reads `deslag.toml`), and it does not build or run tests. Fix what deslag +reports by its advice; only the owner changes its limits or adds an `allow` entry. + +Run `make core-roundtrip-test` for corert (especially `mx/core` work) before pushing core work. +`make api-test` runs the api/impl suite + examples; `make api-roundtrip` and the gen/audit gates run +separately. `make test-all` runs every C++ suite at once (core roundtrip + unit + api-test + +api-roundtrip), the deep gate CI runs on Linux (see `make help` / CI). + +Adding/removing a `data/` file: bump the pinned count in `CoreRoundtripTest.cpp`, run `make audit` +(regenerates `corpus.xml` + `*.features.xml`), confirm round-trip via `make core-roundtrip-test`. + +`ApiLoadSmokeTest` proves a file imports without crashing, not that the data is correct. The +read->write->read gate (`make api-roundtrip` / `roundtrip-baseline.txt`) is the correctness check; +pin a fixture there to defend a feature. + +`make wasm-test` builds `mx::api` for Emscripten and runs the examples under Node: CI's check that mx +still compiles and works client-side in a browser (a consumer, denigma, depends on this; issue +#386). `make backend-pugixml-test` builds mx against a stock pugixml (`MX_XML_BACKEND=pugixml`, see +the README's "Bringing your own XML library") and runs the core suites: CI's check that the XML +backend option works. `make symbol-check` only applies to the default vendored build. Look at what will run in CI `.github/workflows/ci.yaml` and anticipate issues there when coding locally. Code coverage is not part of the normal CI run; trigger it on demand with a `/coverage` diff --git a/CMakeLists.txt b/CMakeLists.txt index b865c29bd..038e04f39 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -6,7 +6,7 @@ project(mx CXX) # + the mx::xml seam over the XML library (src/private/mx/xml, backed by mx's # private copy of pugixml, mxpugi, unless MX_XML_BACKEND says otherwise), plus # the corert/unit test binaries. -# See AGENTS.md, docs/ai/design/mx-core-plan.md and docs/ai/design/xml-backend.md. +# See AGENTS.md and docs/ai/design/xml-backend.md. set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -108,7 +108,7 @@ set(PRIVATE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/src/private") set(PUBLIC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/src/include") # mxpugi: the XML library behind mx::core's XML seam in the default -# (vendored) mode (plan §2.1 chose pugixml). It is pugixml, vendored at +# (vendored) mode (mx::core uses pugixml directly). It is pugixml, vendored at # src/private/mxpugi by scripts/vendor_pugixml.py with its namespace renamed # from `pugi` to `mxpugi` (stock pugiconfig). The private copy exists so that # mx does not define any `pugi::` symbol: a consumer that links its own diff --git a/Makefile b/Makefile index 6bd4b293d..d0201da86 100644 --- a/Makefile +++ b/Makefile @@ -55,7 +55,8 @@ GEN_PY := $(shell find gen -name '*.py' ! -name quality.py | sort) GEN_QUALITY_FLOOR ?= 84.5 GEN_LINT_FLOOR ?= 9.29 -# deslag, a Rust CLI. No target uses it yet. The version is pinned once, in the +# deslag, a Rust CLI that lints prose (deslag.toml picks the files). fmt-check +# runs it. The version is pinned once, in the # Dockerfile. The mx-sdk image installs it and sets DESLAG to it. Natively, # `make deslag` installs it under build/ with a cargo home of its own, so it # does not use or change the deslag or the cargo home in your home directory. @@ -178,9 +179,9 @@ help: @echo '' @echo ' Housekeeping:' @echo ' make doctor Report which targets this machine can run, with Docker or natively.' - @echo ' make deslag Native only: install deslag $(DESLAG_VERSION) at $(DESLAG_HOME) (needs cargo). No target uses it yet.' + @echo ' make deslag Native only: install deslag $(DESLAG_VERSION) at $(DESLAG_HOME) (needs cargo). fmt-check runs it.' @echo ' make fmt Format C++ under src/ with clang-format.' - @echo ' make fmt-check clang-format gate (checks formatting; builds nothing).' + @echo ' make fmt-check clang-format and deslag gate (checks formatting and prose; builds nothing).' @echo ' make sdk Build the mx-sdk Docker toolchain image.' @echo ' make vendor-pugixml VERSION=x.y Refresh src/private/mxpugi from a pugixml release (runs on the host).' @echo ' make clean Remove the build/ tree and gen build artifacts.' @@ -538,6 +539,8 @@ fmt: fmt-check: @$(FIND_CPP) | xargs -r $(CLANG_FORMAT) --dry-run --Werror + @test -x '$(DESLAG)' || { echo "No deslag at $(DESLAG). Run: make deslag"; exit 1; } + @'$(DESLAG)' check @echo "fmt-check passed." gen: gen-cpp gen-go gen-c gen-schema diff --git a/Package.swift b/Package.swift index 90a935c83..2c8b0aa00 100644 --- a/Package.swift +++ b/Package.swift @@ -10,7 +10,7 @@ import PackageDescription // machine-generated binary-default manifest (an XCFramework binaryTarget) // interpolated from .github/release/Package.swift.template by the // release-build workflow. That manifest exists only at tagged release commits, -// never at a tip of main. See docs/ai/design/release-process.md. +// never at a tip of main. // // KEEP IN SYNC: the target below must stay byte-for-byte identical to // mxSourceTarget in .github/release/Package.swift.template, so a release built diff --git a/deslag.toml b/deslag.toml new file mode 100644 index 000000000..2b35949de --- /dev/null +++ b/deslag.toml @@ -0,0 +1,128 @@ +schema_version = 2 +deslag_version = "0.0.6" + +[md] +globs = ["/AGENTS.md", "/docs/**/*.md"] + +# The byte budget. +[md.lints.max_size_bytes] +value = 16000 + +# The limit on bold, italics and ALL CAPS. +[md.lints.max_emphasis] +free_spans = 2 +max_percent = 1 + +# The characters the file must not hold, such as the em dash. +[md.lints.banned_chars] + +[md.lints.banned_chars.groups] +arrows = true +bidi = true +box_drawing = true +bullets = true +checks = true +dashes = true +ellipsis = true +emoji = true +invisible = true +math = true +quotes = true +section = true +spaces = true + +# The phrases the file must not hold, which the config lists. +[md.lints.banned_phrases] + +[md.lints.banned_phrases.ban] +"deliberately not" = "say what it does; what it does not do rarely needs saying" +"not silently" = "say what it does instead, such as the error it reports" +"never silently" = "say what it does instead, such as the error it reports" +load-bearing = 'say it in plain English, such as "important", "fundamental" or "structural"' +"lands on" = 'say what happens, such as "is included in" or "is released by"' +"it lands" = 'say what happens, such as "it is merged" or "it ships"' +byte-identical = 'say "identical", or name the check that shows it' + +# The longest a paragraph or list item may be. +[md.lints.density] +max_item_chars = 350 +max_paragraph_chars = 700 + +# That a sentence negates its verb, not its object. +[md.lints.verbs_no_nouns] + +# That a negation drops `at all`. +[md.lints.not_at_all] + +# That a sentence names what its verb did not find, not `nothing` or `none`. +[md.lints.verbs_nothing] + +# That a possessive noun is not padded with `own`. +[md.lints.possessive_own] + +[[md.overrides]] +globs = ["/AGENTS.md"] + +# The index of the repo that the file must hold. +[md.overrides.lints.repo_layout] +heading = "Repository layout" +max_entries = 30 +max_width = 100 +min_entries = 5 + +# mx::api headers and sources. Generated code (src/private/mx/core/generated) is +# never onboarded. +[cpp] +globs = ["/src/include/mx/api/**/*.h", "/src/private/mx/api/**/*.h", "/src/private/mx/api/**/*.cpp"] +exclude = ["/src/private/mx/core/generated/**/*.h", "/src/private/mx/core/generated/**/*.cpp"] + +[cpp.lints.max_emphasis] +free_spans = 2 +max_percent = 1 + +[cpp.lints.banned_chars] + +[cpp.lints.banned_chars.groups] +arrows = true +bidi = true +box_drawing = true +bullets = true +checks = true +dashes = true +ellipsis = true +emoji = true +invisible = true +math = true +quotes = true +section = true +spaces = true + +[cpp.lints.banned_phrases] + +[cpp.lints.banned_phrases.ban] +"deliberately not" = "say what it does; what it does not do rarely needs saying" +"not silently" = "say what it does instead, such as the error it reports" +"never silently" = "say what it does instead, such as the error it reports" +load-bearing = 'say it in plain English, such as "important", "fundamental" or "structural"' +"lands on" = 'say what happens, such as "is included in" or "is released by"' +"it lands" = 'say what happens, such as "it is merged" or "it ships"' +byte-identical = 'say "identical", or name the check that shows it' + +[cpp.lints.density] +max_item_chars = 350 +max_paragraph_chars = 700 + +[cpp.lints.verbs_no_nouns] + +[cpp.lints.not_at_all] + +[cpp.lints.verbs_nothing] + +[cpp.lints.possessive_own] + + +# The table of common transpositions reads best as one block, which is longer than a paragraph may +# be. +[[cpp.overrides]] +globs = ["/src/include/mx/api/TransposeData.h"] +lints.density.max_paragraph_chars = 850 diff --git a/docs/ai/design/api-roundtrip-classifier.md b/docs/ai/design/api-roundtrip-classifier.md index aebe49a99..9a1d2f3b5 100644 --- a/docs/ai/design/api-roundtrip-classifier.md +++ b/docs/ai/design/api-roundtrip-classifier.md @@ -1,37 +1,37 @@ -# api round-trip failure classifier — diff design +# api round-trip failure classifier - diff design Design notes for the `audit/classify.py` classifier (issue #211, Phase 2 of #208). -This document explains *how the classifier diffs the expected/actual dump pairs* +This document explains how the classifier diffs the expected/actual dump pairs and why. It supersedes the original "walk both trees in parallel until the first mismatch" sketch in the #211 issue body, which has a structural defect described below. ## Update (#212): measurement-only classification -The classifier was reworked to be grounded **only** in what the round-trip +The classifier was reworked to be grounded only in what the round-trip measurably did to each file. The earlier version cross-referenced `data/api.features.xml` to label drops as "by-design" (category B) vs. bug (category G) and to recognize enum/attribute gaps (D/E). That `support` field is -hand-authored opinion — a *prediction* of round-trip behavior, not a measurement -— and it can be wrong (part-group was marked `full` yet dropped data; see #224). +hand-authored opinion - a prediction of round-trip behavior, not a measurement, +and it can be wrong (part-group was marked `full` yet dropped data; see #224). Keying classification on a fallible prediction made the worklist untrustworthy. What changed: -- **No `api.features.xml`.** The support cross-reference (old Layer 3 and the +- No `api.features.xml`. The support cross-reference (old Layer 3 and the B/D/E/G category logic) is removed. Whether a drop is intended is a present-day human decision (#214), not a property the classifier asserts. -- **Categories → signatures.** Every difference is reduced to a signature: +- Categories -> signatures. Every difference is reduced to a signature: `drop:`, `add:`, `value:`, `value:@`, - `attr:@`, `reorder:`. A file's **distance** to passing is - its count of *unique* signatures (a tag dropped a thousand times is one + `attr:@`, `reorder:`. A file's distance to passing is + its count of unique signatures (a tag dropped a thousand times is one `drop:` signature, distance 1). -- **Statuses** are just `PASS` / `FAIL` / `CRASH` (the three pipeline codes fold +- Statuses are just `PASS` / `FAIL` / `CRASH` (the three pipeline codes fold into `CRASH`). -- **Candidates and the worklist.** A FAIL with no `reorder:` signature is a - *candidate* (reorders are expected `mx::api` behavior, to be absorbed in test +- Candidates and the worklist. A `FAIL` with no `reorder:` signature is a + candidate (reorders are expected `mx::api` behavior, to be absorbed in test normalization later, #214). The worklist ranks each signature by the number of - candidate files it is the *sole* blocker of (fixing it flips them green now), + candidate files it is the sole blocker of (fixing it flips them green now), then by total candidate files blocked. Layers 1 (multiset for drops/adds) and 4 (alignment, now also used for @@ -42,13 +42,13 @@ rationale; the current schema is at the end of this section. ## The defect in the naive design Both `compareElements` (`src/private/mxtest/corert/Compare.cpp`) and the original -#211 step 3b do the same thing: a **positional, paired, depth-first walk** of the -two trees that stops at the **first** divergence. That is correct as a pass/fail -gate (corert only needs to know *whether* the documents differ), but it is wrong -as an *analysis* tool. +#211 step 3b do the same thing: a positional, paired, depth-first walk of the +two trees that stops at the first divergence. That is correct as a pass/fail +gate (corert only needs to know whether the documents differ), but it is wrong +as an analysis tool. `mx::api` is a deliberate subset of MusicXML: it drops whole subtrees and reorders -children **by design**. The dominant failure signal is therefore a *deletion* — +children by design. The dominant failure signal is therefore a deletion - an element present in `expected` that never appears in `actual`. A positional walk cannot survive a deletion: @@ -59,34 +59,34 @@ actual children: [part-list] [part] [???] ... <- credit droppe The moment `actual` is missing one child, the two child-lists shift out of alignment, and every subsequent sibling is compared against the wrong partner. -You can only trust the **first** reported divergence; everything after it is noise. +You can only trust the first reported divergence; everything after it is noise. A single drop poisons the rest of the file. This is a well-documented property of diffing: a naive index-by-index comparison breaks because one insert/delete shifts every later index, so the differ keeps matching content at positions that no longer correspond. The principled fixes are -**alignment** (LCS / Myers) or, when position is irrelevant, an **order-free -multiset** comparison. See the research summary at the end of this document. +alignment (LCS / Myers) or, when position is irrelevant, an order-free +multiset comparison. See the research summary at the end of this document. ## What we actually need -The classifier's job is *not* to produce a minimal edit script. It is to answer: +The classifier's job is not to produce a minimal edit script. It is to answer: -1. **Which element classes (tag names) are present in `expected` but missing from - `actual`?** — the complete set, not just the first. -2. **How many *distinct* element classes are missing per file?** — the headline +1. Which element classes (tag names) are present in `expected` but missing from + `actual`? - the complete set, not just the first. +2. How many distinct element classes are missing per file? - the headline ranking metric. -3. **Which files are missing exactly one distinct class** (once or many times)? — - the low-hanging fruit: a single api addition flips them to PASS. -4. A cheap **"distance to passing"** scalar for secondary tie-breaking. +3. Which files are missing exactly one distinct class (once or many times)? - + the low-hanging fruit: a single api addition flips them to `PASS`. +4. A cheap "distance to passing" scalar for secondary tie-breaking. -None of these require positions or a minimal edit script. They require a *complete -inventory of deletions*, which is exactly what a multiset difference gives in +None of these require positions or a minimal edit script. They require a complete +inventory of deletions, which is exactly what a multiset difference gives in linear time, fully invariant to reordering. Exact order-insensitive structural diffing (unordered tree edit distance) is NP-complete, and ordered tree edit distance (Zhang-Shasha / RTED / APTED) is -O(n³)/O(n²) — both far more than this problem needs. We deliberately do **not** +O(n³)/O(n²) - both far more than this problem needs. We deliberately do not use them. ## The layered design @@ -94,10 +94,10 @@ use them. All layers operate on the two normalized dump documents (`*.expected.xml`, `*.actual.xml`) using only the Python standard library (`xml.etree.ElementTree`, `collections.Counter`, `difflib`). No `lxml`, `xmldiff`, -`zss`, or `apted` dependency is introduced — the multiset primitive covers the need +`zss`, or `apted` dependency is introduced - the multiset primitive covers the need in O(n). -### Layer 1 — multiset tag diff (primary signal) +### Layer 1 - multiset tag diff (primary signal) Recurse both trees, building a `Counter` of element tag names: @@ -116,15 +116,15 @@ multiset of element classes the round-trip dropped. From it: | field | meaning | |---|---| -| `missing_elements` | `sorted(missing)` — the distinct dropped tag names | -| `missing_element_counts` | `dict(missing)` — how many of each were dropped | -| `distinct_missing_count` | `len(missing)` — **the headline ranking metric** | -| `added_elements` | `sorted(added)` — spurious tags the api invented (bug signal) | +| `missing_elements` | `sorted(missing)` - the distinct dropped tag names | +| `missing_element_counts` | `dict(missing)` - how many of each were dropped | +| `distinct_missing_count` | `len(missing)` - the headline ranking metric | +| `added_elements` | `sorted(added)` - spurious tags the api invented (bug signal) | -This is O(n), reorder-invariant, and enumerates **every** dropped class — it does +This is O(n), reorder-invariant, and enumerates every dropped class - it does not stop at the first one. It is the direct fix for the cascade defect. -### Layer 2 — path-qualified multiset (attribution) +### Layer 2 - path-qualified multiset (attribution) Bare tag names are ambiguous (`` under `` vs. elsewhere). For `blocking_features` attribution and to line up with `first_divergence_path`, build @@ -132,40 +132,40 @@ a second multiset keyed by `parent/tag` (or the full root-to-node path). The headline `distinct_missing_count` stays on bare tags; path qualification is used when mapping a drop back to an `api.features.xml` feature. -### Layer 3 — cross-reference `api.features.xml` *(REMOVED in #212; historical)* +### Layer 3 - cross-reference `api.features.xml` (REMOVED in #212; historical) This layer was removed: classification no longer consults `api.features.xml`. The text below is retained only to explain what the categories used to mean. For each missing tag, the old version looked up `support` in `api.features.xml`: -- **Category B (drop-only)** becomes *provable across the whole file*: it holds iff - **every** tag in `missing` has `support="none"`. The naive walk could never - establish this — it only saw the first drop. This is the single biggest +- Category B (drop-only) becomes provable across the whole file: it holds iff + every tag in `missing` has `support="none"`. The naive walk could never + establish this - it only saw the first drop. This is the single biggest correctness win of the redesign. - A missing tag with `support="full"` or `"partial"` is a genuine correctness bug - or a partial-drop — surfaced explicitly instead of being hidden behind whatever + or a partial-drop - surfaced explicitly instead of being hidden behind whatever the first divergence happened to be. -### Layer 4 — sequence alignment per parent (reorder + paired value/attr) +### Layer 4 - sequence alignment per parent (reorder + paired value/attr) `difflib.SequenceMatcher` over the two child-tag sequences earns its keep twice: -- **Reorder** (`reorder:`): a parent whose child *multiset* is equal but - whose child *sequence* differs is a pure permutation. -- **Paired value/attr** (`value:`/`attr:`): recursing only the matcher's `equal` +- Reorder (`reorder:`): a parent whose child multiset is equal but + whose child sequence differs is a pure permutation. +- Paired value/attr (`value:`/`attr:`): recursing only the matcher's `equal` blocks pairs surviving elements across drops without desyncing, so text and attribute differences are compared between genuinely corresponding nodes (never across mismatched tags). This is how value/attr diffs survive a sibling drop. -It is **not** used for the deletion inventory (Layer 1's multiset already has +It is not used for the deletion inventory (Layer 1's multiset already has that). Stdlib, zero dependency. -### Layer 5 — edit-distance scalar (secondary) +### Layer 5 - edit-distance scalar (secondary) Use `sum(missing.values())` (total dropped instances) as a cheap "distance to -passing." It is a *secondary* tie-breaker only. We do **not** key ranking on it: -a scalar conflates "one big dropped subtree" with "many tiny diffs" and names no -feature. Full tree edit distance would also pull in a heavyweight dependency for +passing." It is a secondary tie-breaker only. We do not key ranking on it: +a scalar conflates "one big dropped subtree" with "many tiny diffs" and does not +name a feature. Full tree edit distance would also pull in a heavyweight dependency for no added decision value here. ## Ranking output (the worklist) @@ -177,11 +177,11 @@ prioritized api-addition worklist. Two refinements from the regression-triage literature (e.g. BuildSheriff, ICSE'22: failures cluster around a few root causes; fixing one unblocks a cluster): -1. **Rank by files *unblocked*, not occurrence count.** A tag appearing 10,000× - in one file is lower yield than a tag that is the *sole* blocker of 200 files. +1. Rank by files unblocked, not occurrence count. A tag appearing 10,000 times + in one file is lower yield than a tag that is the sole blocker of 200 files. The y-axis is "count of files whose single blocking class is tag X." -2. **Histogram on the categorical root cause (missing tag), not the edit-distance - scalar.** Keep the scalar as a within-cluster severity tie-breaker. +2. Histogram on the categorical root cause (missing tag), not the edit-distance + scalar. Keep the scalar as a within-cluster severity tie-breaker. This belongs to Phase 3 (#212) ranking, but the per-file fields it needs (`distinct_missing_count`, `missing_elements`, single-blocker flag) are emitted @@ -217,10 +217,10 @@ document). So the dump step (#210) writes a tiny `.status` sidecar next to the expected file recording the exact code. The classifier reads it into `crash_kind` (with `status = "CRASH"`); when the sidecar is absent, `status` is still `"CRASH"` and `crash_kind` is `null`. The presence/absence of `.actual.xml` -remains the FAIL-vs-CRASH signal; the `.status` sidecar only refines *which* +remains the `FAIL`-vs-`CRASH` signal; the `.status` sidecar only refines which crash. The sidecar lives in the gitignored dump dir and is never checked in. -### Category G — `support="full"`/`"partial"` drops *(REMOVED in #212)* +### Category G - `support="full"`/`"partial"` drops (REMOVED in #212) Category G (and the B/D/E categories) existed only because classification used to read `api.features.xml`. With that cross-reference gone, every dropped tag is just @@ -228,7 +228,7 @@ a `drop:` signature regardless of any prior belief about whether it was "supported"; the supported-vs-by-design judgment is a present-day human call (#214), not the classifier's. `part-group` (marked `support="full"` yet dropped; #224) was the case that proved the `support` field could not be trusted as a -classification input — which is what motivated removing it. The drop is still +classification input - which is what motivated removing it. The drop is still reported, now as `drop:part-group`, ranked purely by how many files it blocks. ## Dependency decision @@ -250,52 +250,52 @@ claim from training knowledge that the fetched sources did not directly confirm ### Alignment vs. positional walk - A naive index-by-index walk cascades after one insert/delete; Myers models diff as shortest-path search on an edit graph where diagonal (match) moves are free, so - shared content anchors the alignment. *(High — jcoglan.com/2017/02/12/the-myers-diff-algorithm-part-1/; xmailserver.org/diff2.pdf)* -- Microsoft XML Diff documents three child-matching modes — By Index (naive), LCS - (survives ins/del), Unordered (set-like). *(Med-high — documentation.help/Microsoft-XML-Diff)* + shared content anchors the alignment. (High - jcoglan.com/2017/02/12/the-myers-diff-algorithm-part-1/; xmailserver.org/diff2.pdf) +- Microsoft XML Diff documents three child-matching modes - By Index (naive), LCS + (survives ins/del), Unordered (set-like). (Med-high - documentation.help/Microsoft-XML-Diff) ### Tree edit distance (decided against) - Zhang-Shasha O(n⁴)/O(nm); Klein O(n³ log n); Demaine'07 O(n³); RTED O(n³)/O(n²); - APTED O(n³)/O(n²) with an edit *mapping* (unmapped source nodes = deletions). - *(High — erikdemaine.org/papers/TreeEdit_ICALP2007/paper.pdf; arxiv 1201.0230; - dl.acm.org/doi/10.1145/2699485; github.com/DatabaseGroup/apted)* -- **Unordered** tree edit distance is NP-complete / MAX-SNP-hard even for binary - trees over a 2-symbol alphabet. *(High — Bille survey, sciencedirect - S0304397505000174; arxiv 2108.00953)* + APTED O(n³)/O(n²) with an edit mapping (unmapped source nodes = deletions). + (High - erikdemaine.org/papers/TreeEdit_ICALP2007/paper.pdf; arxiv 1201.0230; + dl.acm.org/doi/10.1145/2699485; github.com/DatabaseGroup/apted) +- Unordered tree edit distance is NP-complete / MAX-SNP-hard even for binary + trees over a 2-symbol alphabet. (High - Bille survey, sciencedirect + S0304397505000174; arxiv 2108.00953) ### XML-specific algorithms - Chawathe et al. 1996 (basis of Python `xmldiff`): match phase + edit-script phase, - ordered, emits INSERT/UPDATE/MOVE/DELETE — enumerates deletions and reports a - reorder as one MOVE. *(High on ops; exact complexity bound UNVERIFIED — primary - PDF 503'd. sigmodrecord.org/1996/06/24/...; github.com/cfpb/xtdiff)* + ordered, emits INSERT/UPDATE/MOVE/DELETE - enumerates deletions and reports a + reorder as one MOVE. (High on ops; exact complexity bound UNVERIFIED - primary + PDF 503'd. sigmodrecord.org/1996/06/24/...; github.com/cfpb/xtdiff) - X-Diff: unordered model (reorder is a non-change), bottom-up signature hashing + - min-cost sibling matching; harder to compute, no MOVE op. *(Ordered/unordered: - high — pages.cs.wisc.edu/~yuanwang/xdiff.html; "no MOVE": UNVERIFIED)* + min-cost sibling matching; harder to compute, no MOVE op. (Ordered/unordered: + high - pages.cs.wisc.edu/~yuanwang/xdiff.html; "no MOVE": UNVERIFIED) ### Sequence alignment & multiset - Myers O(ND) powers git/GNU diff; per-parent LCS over child sequences is an - established lightweight alternative to TED (XMLMind). *(High — xmailserver.org/diff2.pdf; - xmlmind.com/.../xmlDiff_algorithm.html)* + established lightweight alternative to TED (XMLMind). (High - xmailserver.org/diff2.pdf; + xmlmind.com/.../xmlDiff_algorithm.html) - `collections.Counter` is a multiset; `a - b` keeps only positive counts = elements in `a` not covered by `b`, directly enumerable; building is O(n), - set-ops O(n+m). *(High on semantics — docs.python.org/3/library/collections.html; - the explicit O(n) is standard reasoning, not doc-quoted → UNVERIFIED as a doc claim)* + set-ops O(n+m). (High on semantics - docs.python.org/3/library/collections.html; + the explicit O(n) is standard reasoning, not doc-quoted -> UNVERIFIED as a doc claim) ### Python libraries - `xmldiff` 3.0 (2026-06-11, MIT, active): Chawathe; enumerable `DeleteNode`/ - `MoveNode`/… actions; **requires lxml (C ext)**. -- `zss` (2018, BSD): Zhang-Shasha; **scalar distance only, no edit script**. + `MoveNode`/... actions; requires lxml (C ext). +- `zss` (2018, BSD): Zhang-Shasha; scalar distance only, no edit script. - `apted` (2017, MIT, pure Python): edit mapping (deletions = unmapped source nodes); needs bracket-notation input. - `lxml`: no semantic diff, only C14N canonicalization. - stdlib `difflib.SequenceMatcher` (opcodes) and `collections.Counter` (multiset): - zero-dependency, both enumerate deletions. *(All high — verified against live - PyPI/GitHub/ReadTheDocs)* + zero-dependency, both enumerate deletions. (All high - verified against live + PyPI/GitHub/ReadTheDocs) ### Yield-based ranking - BuildSheriff (ICSE'22, 163K builds): failures cluster around a few root causes; - triaging by shared cause saves cost — validates "many files, few blockers." - *(Med-high — chenbihuan.github.io/paper/icse22-zhang-buildsheriff.pdf)* -- Rank by **files unblocked**, not occurrence count; histogram on categorical root - cause, with edit distance only as a secondary severity scalar. *(Synthesized from - the triage literature above + frequency×impact practice)* + triaging by shared cause saves cost - validates "many files, few blockers." + (Med-high - chenbihuan.github.io/paper/icse22-zhang-buildsheriff.pdf) +- Rank by files unblocked, not occurrence count; histogram on categorical root + cause, with edit distance only as a secondary severity scalar. (Synthesized from + the triage literature above + frequency-times-impact practice) diff --git a/docs/ai/design/mx-core-gpt-5.0.md b/docs/ai/design/mx-core-gpt-5.0.md deleted file mode 100644 index 009a9905f..000000000 --- a/docs/ai/design/mx-core-gpt-5.0.md +++ /dev/null @@ -1,1114 +0,0 @@ -# C++ `mx/core` design - -Status: design proposal for the generated C++20 `mx::core` product. - -The Go and C targets proved the generator pipeline: XSD -> IR -> Plates -> target-owned Mustache -works, the press is language-agnostic, and the corert flow can be made green from generated code. -They also proved what the C++ target must **not** copy. Go and C are deliberately pragmatic test -bindings: they preserve document order with an ordered child list, parse values leniently, and -expose plain data. The C++ target is the real product and should be a stricter, deeper abstraction. - -## Position - -Generate a valid-by-construction, order-faithful MusicXML model. - -The public C++ API should make invalid MusicXML states unreachable through normal API use: - -- no public mutable fields; -- no public default constructors for model objects whose schema content is required; -- required attributes and required children appear in constructors or `make` factories; -- optional schema members use `std::optional` behind accessors; -- repeated members are stored privately and exposed as read-only spans; -- schema choices use `std::variant`, not nullable sibling fields; -- XML parsing validates structure and required content before producing a model object; -- import leniency may clamp out-of-range numbers, but the object produced is still valid. - -C++ cannot protect against undefined behavior, memory scribbles, malicious casts, or every possible -moved-from misuse. The contract is: the generated API does not provide an operation that creates an -invalid serializable model object. - -## Lessons from Go and C - -What to keep: - -1. **One source of truth.** All schema facts must still come from the Plates. C++ is a target - directory: `gen/cpp/config.toml` plus `gen/cpp/templates/`. Do not add C++ knowledge to - `gen/*.py`. -2. **Order fidelity matters.** A MusicXML measure interleaves `note`, `backup`, `direction`, - `attributes`, and other music-data elements. A flat field per element is not enough. -3. **Generated version constants matter.** The C++ runtime should emit `SupportedMusicXMLVersion` - from `Plates.schema_version`, just like Go and C now do. -4. **Unknown names are errors.** With version gating, an unknown element or attribute is a generator - or input problem, not data to preserve silently. -5. **Numeric clamping is an import policy.** The clamp steps now live on the Plates. C++ should use - that data exactly once in its value parsers. - -What not to keep: - -1. **No multi-null child structs.** Go/C use `NoteChild{Pitch: &p}` because they lack a good sum - type in the chosen style. C++ has `std::variant`; use it. -2. **No public structs as the product API.** Go/C are harness bindings. C++ should hide invariants - behind small interfaces. -3. **No always-flat child list.** The C++ target should consume `ComplexPlate.content`, not just - `elements`. Pure sequences should become sequence-shaped classes; repeated choices should become - ordered variants. -4. **No enum fallback to the first value for user construction.** Unknown enum input should fail. - Clamping numbers can be compatible with a valid model; inventing enum values is not. - -## C++ feature choices - -| Feature | Use | Rule | -|------------------------------|---------------------------------------------------------------------|-------------------------------------------------------------------------------------| -| C++20 | Target language level | Raise CMake to C++20 when this target lands. | -| `class` with private members | Generated model types | Default for all complex and constrained value types. | -| `enum class` | Internal tags | Do not expose raw enums as constructible domain values. Wrap them in value classes. | -| `std::variant` | XML choices, unions, document root | Exactly-one by construction. | -| `std::optional` | Optional attributes/elements and defaulted/fixed attribute presence | Never for required schema content. | -| `std::vector` | Repeated homogeneous content and repeated choice groups | Kept private; expose `std::span`. | -| `std::unique_ptr` | Alternatives inside repeated heterogeneous choice vectors | Avoids a huge `std::variant` object per child. | -| `std::span` | Read-only views of repeated content | No mutable vector exposure. | -| `std::string_view` | Parse/format APIs and wire names | Avoids unnecessary string copies at API boundaries. | -| `constexpr std::array` | Wire literal tables | No dynamic enum maps unless profiling proves need. | -| `std::from_chars` | Numeric lexical parsing | Strict, locale-free parsing. | -| non-polymorphic inheritance | Only for IR `derived` complex types | Schema extension only; no virtual model hierarchy. | -| `Result` runtime type | Parse/factory errors | C++20 has no `std::expected`; provide a small target runtime `Result`. | - -Avoid: - -- raw owning pointers; -- `new` / `delete` in generated code; -- public mutable containers; -- runtime polymorphism for every element; -- `std::shared_ptr` by default; -- `std::regex` in headers; -- template-heavy generated code. - -## Output layout - -Recommended generated layout under `src/private/mx/core/`: - -```text -mx/core/ - Document.h / Document.cpp - Result.h - Error.h - Decimal.h / Decimal.cpp - Xml.h / Xml.cpp - UpDown.h / UpDown.cpp - FontSize.h / FontSize.cpp - Pitch.h / Pitch.cpp - PartwiseMeasure.h / PartwiseMeasure.cpp - ... one header/source pair per generated type ... - Sources.cmake -``` - -Headers should contain type declarations, small accessors, and inline trivial factories. Parsing, -serialization, string tables, pattern checks, and visitors should live in `.cpp` files. Compile time -is the first optimization priority; do not dump large parse functions into headers. - -## Error and result shape - -The runtime support can define generic infrastructure, but generated code should use concrete result -spelling. Rendered code should look like this at call sites: - -```cpp -namespace mx::core -{ - -enum class ErrorCode -{ - unknownElement, - unknownAttribute, - missingRequiredElement, - missingRequiredAttribute, - wrongElementOrder, - tooManyElements, - invalidValue, - unsupportedVersion, -}; - -struct Error -{ - ErrorCode code; - std::string path; - std::string message; -}; - -Result parsePitch(const ezxml::XElement &element); -Result parseDocument(const ezxml::XDoc &doc); - -} // namespace mx::core -``` - -The generated functions should accumulate path context -(`/score-partwise/part[0]/measure[2]/note[1]`) so corert failures are diagnosable. The old -`bool fromXDoc(std::ostream&, ...)` shape can be kept as a thin adapter; it should not be the core -API. - -## Value type shapes - -### Closed enum values: wrapper class, not a raw enum - -A raw C++ enum can be fabricated with `static_cast`. Use a small value class with a private tag -constructor. Rendered `up-down` should look like this: - -```cpp -#pragma once - -#include "mx/core/Result.h" - -#include -#include - -namespace mx::core -{ - -class UpDown final -{ - public: - enum class Tag : std::uint8_t - { - up, - down, - }; - - static constexpr UpDown up() noexcept { return UpDown(Tag::up); } - static constexpr UpDown down() noexcept { return UpDown(Tag::down); } - - static Result parse(std::string_view text); - - constexpr Tag tag() const noexcept { return tag_; } - constexpr bool isUp() const noexcept { return tag_ == Tag::up; } - constexpr bool isDown() const noexcept { return tag_ == Tag::down; } - - std::string_view wire() const noexcept; - - private: - explicit constexpr UpDown(Tag tag) noexcept : tag_(tag) {} - - Tag tag_; -}; - -} // namespace mx::core -``` - -The `.cpp` keeps the wire table out of dependent translation units: - -```cpp -#include "mx/core/UpDown.h" - -#include - -namespace mx::core -{ -namespace -{ -constexpr std::array kWire = {"up", "down"}; -} - -Result UpDown::parse(std::string_view text) -{ - if (text == "up") - { - return UpDown::up(); - } - if (text == "down") - { - return UpDown::down(); - } - return Error{ErrorCode::invalidValue, {}, "invalid up-down value"}; -} - -std::string_view UpDown::wire() const noexcept -{ - return kWire[static_cast(tag_)]; -} - -} // namespace mx::core -``` - -### Numeric wrappers: strict construction, explicit import clamping - -The Plates already carry clamp policy. Use it for XML import, not for ordinary construction. -Rendered `midi-channel` should look like this: - -```cpp -#pragma once - -#include "mx/core/Result.h" - -#include - -namespace mx::core -{ - -class MIDIChannel final -{ - public: - static Result make(int value); - static Result parse(std::string_view text); - static MIDIChannel importClamp(int value) noexcept; - - constexpr int value() const noexcept { return value_; } - std::string toString() const; - - private: - explicit constexpr MIDIChannel(int value) noexcept : value_(value) {} - - int value_; -}; - -} // namespace mx::core -``` - -```cpp -#include "mx/core/MIDIChannel.h" -#include "mx/core/NumberParse.h" - -namespace mx::core -{ - -Result MIDIChannel::make(int value) -{ - if (value < 1 || value > 16) - { - return Error{ErrorCode::invalidValue, {}, "midi-channel must be in [1, 16]"}; - } - return MIDIChannel(value); -} - -MIDIChannel MIDIChannel::importClamp(int value) noexcept -{ - if (value < 1) - { - value = 1; - } - if (value > 16) - { - value = 16; - } - return MIDIChannel(value); -} - -Result MIDIChannel::parse(std::string_view text) -{ - const auto parsed = parseInteger(text); - if (!parsed) - { - return parsed.error(); - } - return MIDIChannel::make(parsed.value()); -} - -std::string MIDIChannel::toString() const -{ - return formatInteger(value_); -} - -} // namespace mx::core -``` - -The XML parser can call `importClamp` when the target chooses compatibility import. User code gets -`make`, which rejects invalid values. - -### Strings: domain wrappers own validation - -String wrappers should not be aliases. They are the only place length and pattern constraints can be -enforced. - -```cpp -#pragma once - -#include "mx/core/Result.h" - -#include -#include - -namespace mx::core -{ - -class Color final -{ - public: - static Result make(std::string value); - static Result parse(std::string_view text); - - const std::string &value() const noexcept { return value_; } - std::string_view wire() const noexcept { return value_; } - - private: - explicit Color(std::string value) : value_(std::move(value)) {} - - std::string value_; -}; - -} // namespace mx::core -``` - -Pattern validation belongs in `.cpp`. If the XSD regex subset needs custom handling (`\c` appears in -MusicXML), hide it in `XmlPattern.cpp`; do not generate ad-hoc regex logic into every header. - -### Unions: `std::variant` - -Rendered `font-size` should be a tiny sum type. This is strictly better than Go/C's hand-rolled -`Kind` plus fields because `std::variant` cannot hold two members at once. - -```cpp -#pragma once - -#include "mx/core/CSSFontSize.h" -#include "mx/core/Decimal.h" -#include "mx/core/Result.h" - -#include -#include -#include - -namespace mx::core -{ - -class FontSize final -{ - public: - using Value = std::variant; - - static FontSize points(Decimal value) { return FontSize(Value{value}); } - static FontSize css(CSSFontSize value) { return FontSize(Value{value}); } - static Result parse(std::string_view text); - - bool isPoints() const noexcept { return std::holds_alternative(value_); } - bool isCSS() const noexcept { return std::holds_alternative(value_); } - - const Value &value() const noexcept { return value_; } - std::string toString() const; - - private: - explicit FontSize(Value value) : value_(std::move(value)) {} - - Value value_; -}; - -} // namespace mx::core -``` - -```cpp -Result FontSize::parse(std::string_view text) -{ - if (auto decimal = Decimal::parse(text)) - { - return FontSize::points(decimal.value()); - } - if (auto css = CSSFontSize::parse(text)) - { - return FontSize::css(css.value()); - } - return Error{ErrorCode::invalidValue, {}, "invalid font-size"}; -} - -std::string FontSize::toString() const -{ - return std::visit( - [](const auto &member) { return member.toString(); }, - value_); -} -``` - -Open string unions, such as `instrument-sound` after the sounds companion fold, should put the open -string alternative last in parsing, matching the Plates rule. - -## Complex type shapes - -### Pure sequence: fields, not a child vector - -`pitch` is a sequence: `step`, optional `alter`, `octave`. It should not become an ordered vector of -three alternatives. Generate the shape the schema says. - -```cpp -#pragma once - -#include "mx/core/Octave.h" -#include "mx/core/Result.h" -#include "mx/core/Semitones.h" -#include "mx/core/Step.h" - -#include - -namespace mx::core -{ - -class Pitch final -{ - public: - Pitch(Step step, Octave octave) - : step_(step), octave_(octave) - { - } - - static Result make(Step step, std::optional alter, Octave octave); - static Result parseXml(const ezxml::XElement &element); - - const Step &step() const noexcept { return step_; } - const std::optional &alter() const noexcept { return alter_; } - const Octave &octave() const noexcept { return octave_; } - - void setStep(Step value) noexcept { step_ = value; } - void setAlter(std::optional value) { alter_ = std::move(value); } - void setOctave(Octave value) noexcept { octave_ = value; } - - void writeXml(ezxml::XElement &parent, std::string_view tag) const; - - private: - Step step_; - std::optional alter_; - Octave octave_; -}; - -} // namespace mx::core -``` - -A strict parser for `pitch` should be a small generated state machine: - -```cpp -Result Pitch::parseXml(const ezxml::XElement &element) -{ - std::optional step; - std::optional alter; - std::optional octave; - - enum class State - { - start, - afterStep, - afterAlter, - afterOctave, - }; - - State state = State::start; - - for (auto it = element.begin(); it != element.end(); ++it) - { - const auto child = *it; - const auto name = child->getName(); - - if (name == "step" && state == State::start) - { - auto parsed = Step::parse(child->getValue()); - if (!parsed) - { - return parsed.error(); - } - step = parsed.value(); - state = State::afterStep; - } - else if (name == "alter" && state == State::afterStep) - { - auto parsed = Semitones::parse(child->getValue()); - if (!parsed) - { - return parsed.error(); - } - alter = parsed.value(); - state = State::afterAlter; - } - else if (name == "octave" && (state == State::afterStep || state == State::afterAlter)) - { - auto parsed = Octave::parse(child->getValue()); - if (!parsed) - { - return parsed.error(); - } - octave = parsed.value(); - state = State::afterOctave; - } - else - { - return Error{ErrorCode::wrongElementOrder, {}, "invalid child in pitch"}; - } - } - - if (!step) - { - return Error{ErrorCode::missingRequiredElement, {}, "pitch is missing step"}; - } - if (!octave) - { - return Error{ErrorCode::missingRequiredElement, {}, "pitch is missing octave"}; - } - - return Pitch::make(step.value(), alter, octave.value()); -} -``` - -This is the key difference from Go/C: C++ should consume the content tree and enforce sequence -order. - -### Repeated homogeneous members: private vector - -For repeated elements like `beam` in `note`, use private vectors and bounded append APIs. - -```cpp -class Note final -{ - public: - std::span beams() const noexcept { return beams_; } - - Result addBeam(Beam beam) - { - if (beams_.size() == 8) - { - return Error{ErrorCode::tooManyElements, {}, "note has too many beam elements"}; - } - beams_.push_back(std::move(beam)); - return {}; - } - - private: - std::vector beams_; -}; -``` - -If `maxOccurs` is unbounded, `addX` cannot fail for cardinality. If `minOccurs` is at least one, the -constructor or factory takes the first value and the vector never exposes `clear`. - -### Repeated heterogeneous choices: boxed variants - -For measure music-data, order is semantically important and alternatives have very different sizes. -Do not use `std::variant` directly in a vector: every vector slot would -be as large as the largest alternative. Use `std::variant, ...>` and keep the -container private. - -Rendered `partwise-measure` should look like this: - -```cpp -#pragma once - -#include "mx/core/Attributes.h" -#include "mx/core/Backup.h" -#include "mx/core/Barline.h" -#include "mx/core/Bookmark.h" -#include "mx/core/Direction.h" -#include "mx/core/FiguredBass.h" -#include "mx/core/Forward.h" -#include "mx/core/Grouping.h" -#include "mx/core/Harmony.h" -#include "mx/core/Link.h" -#include "mx/core/MeasureText.h" -#include "mx/core/Note.h" -#include "mx/core/Print.h" -#include "mx/core/Result.h" -#include "mx/core/Sound.h" -#include "mx/core/Tenths.h" -#include "mx/core/YesNo.h" - -#include -#include -#include -#include -#include -#include - -namespace mx::core -{ - -class PartwiseMeasure final -{ - public: - using MusicData = std::variant< - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr, - std::unique_ptr>; - - explicit PartwiseMeasure(std::string number) - : number_(std::move(number)) - { - } - - PartwiseMeasure(const PartwiseMeasure &other); - PartwiseMeasure &operator=(const PartwiseMeasure &other); - PartwiseMeasure(PartwiseMeasure &&) noexcept = default; - PartwiseMeasure &operator=(PartwiseMeasure &&) noexcept = default; - - const std::string &number() const noexcept { return number_; } - void setNumber(std::string value) { number_ = std::move(value); } - - const std::optional &text() const noexcept { return text_; } - void setText(std::optional value) { text_ = std::move(value); } - - std::span musicData() const noexcept { return music_data_; } - - void appendNote(Note note) { music_data_.push_back(std::make_unique(std::move(note))); } - void appendBackup(Backup backup) { music_data_.push_back(std::make_unique(std::move(backup))); } - void appendForward(Forward forward) { music_data_.push_back(std::make_unique(std::move(forward))); } - void appendDirection(Direction direction) { music_data_.push_back(std::make_unique(std::move(direction))); } - void appendAttributes(Attributes attributes) { music_data_.push_back(std::make_unique(std::move(attributes))); } - void appendHarmony(Harmony harmony) { music_data_.push_back(std::make_unique(std::move(harmony))); } - void appendFiguredBass(FiguredBass figuredBass) { music_data_.push_back(std::make_unique(std::move(figuredBass))); } - void appendPrint(Print print) { music_data_.push_back(std::make_unique(std::move(print))); } - void appendSound(Sound sound) { music_data_.push_back(std::make_unique(std::move(sound))); } - void appendBarline(Barline barline) { music_data_.push_back(std::make_unique(std::move(barline))); } - void appendGrouping(Grouping grouping) { music_data_.push_back(std::make_unique(std::move(grouping))); } - void appendLink(Link link) { music_data_.push_back(std::make_unique(std::move(link))); } - void appendBookmark(Bookmark bookmark) { music_data_.push_back(std::make_unique(std::move(bookmark))); } - - static Result parseXml(const ezxml::XElement &element); - void writeXml(ezxml::XElement &parent, std::string_view tag) const; - - private: - std::string number_; - std::optional text_; - std::optional implicit_; - std::optional non_controlling_; - std::optional width_; - std::optional id_; - std::vector music_data_; -}; - -} // namespace mx::core -``` - -The copy constructor clones the pointed alternatives. The public API still feels value-like, while -each child slot remains small. - -Serialization dispatch is straightforward and safe: - -```cpp -void PartwiseMeasure::writeXml(ezxml::XElement &parent, std::string_view tag) const -{ - auto element = parent.appendChild(std::string(tag)); - element->appendAttribute("number")->setValue(number_); - - if (text_) - { - element->appendAttribute("text")->setValue(text_->toString()); - } - - for (const auto &item : music_data_) - { - std::visit( - [&](const auto &ptr) - { - using Pointer = std::decay_t; - if constexpr (std::is_same_v>) - { - ptr->writeXml(*element, "note"); - } - else if constexpr (std::is_same_v>) - { - ptr->writeXml(*element, "backup"); - } - else if constexpr (std::is_same_v>) - { - ptr->writeXml(*element, "forward"); - } - else if constexpr (std::is_same_v>) - { - ptr->writeXml(*element, "direction"); - } - }, - item); - } -} -``` - -The rendered file would include all alternatives; the snippet is shortened only after `Direction`. -The shape is the important decision: boxed variant, private vector, typed append methods. - -### Non-repeated choice: by-value variant - -For one-of content that occurs once, prefer by-value alternatives. Example shape for the -`pitch | unpitched | rest` part of `note`: - -```cpp -class Note final -{ - public: - using FullNote = std::variant; - - static Result make(FullNote fullNote, std::optional duration); - - const FullNote &fullNote() const noexcept { return full_note_; } - void setFullNote(FullNote value) { full_note_ = std::move(value); } - - private: - FullNote full_note_; - std::optional duration_; -}; -``` - -This is compact and enforces exactly one full-note alternative. - -### Attributes - -Attributes should be separate from child content. Required attributes are stored by value. Optional, -defaulted, and fixed attributes store presence separately from effective value. - -For defaulted attributes, preserve wire absence but provide an effective getter: - -```cpp -class Barline final -{ - public: - RightLeftMiddle location() const noexcept - { - return location_.value_or(RightLeftMiddle::right()); - } - - bool hasLocationAttribute() const noexcept { return location_.has_value(); } - void setLocationAttribute(RightLeftMiddle value) { location_ = value; } - void clearLocationAttribute() noexcept { location_.reset(); } - - private: - std::optional location_; -}; -``` - -For fixed attributes, do not expose a setter that accepts arbitrary values. Expose presence only: - -```cpp -class Link final -{ - public: - bool hasXlinkTypeAttribute() const noexcept { return has_xlink_type_; } - void writeXlinkTypeAttribute() noexcept { has_xlink_type_ = true; } - void omitXlinkTypeAttribute() noexcept { has_xlink_type_ = false; } - - private: - bool has_xlink_type_ = false; -}; -``` - -The serializer writes the fixed value when the presence flag is true. - -### Empty elements - -Presence-only elements should not become empty structs in parent APIs. In a sequence field, use a -specific marker type only when the element itself needs a type in a `std::variant`. - -```cpp -class Empty final -{ - public: - static constexpr Empty present() noexcept { return Empty(); } - - private: - constexpr Empty() noexcept = default; -}; -``` - -An optional presence-only child in a pure sequence can be `std::optional`. A presence-only -choice alternative can be `Empty` in a variant. - -### Schema derivation - -MusicXML derivation is small and attribute-only. Use non-polymorphic public inheritance only for IR -`derived` types. Do not introduce a virtual base class for all elements. - -```cpp -class EmptyTrillSound -{ - public: - const std::optional &type() const noexcept { return type_; } - void setType(std::optional value) { type_ = std::move(value); } - - private: - std::optional type_; - std::optional accelerate_; - std::optional beats_; -}; - -class Mordent final : public EmptyTrillSound -{ - public: - const std::optional &placement() const noexcept { return placement_; } - void setPlacement(std::optional value) { placement_ = std::move(value); } - - private: - std::optional placement_; -}; -``` - -No virtual destructor is needed because this is not a polymorphic hierarchy and the API should not -traffic in owning base pointers. - -## Document root - -The document has exactly one root. Use a root variant. - -```cpp -#pragma once - -#include "mx/core/Result.h" -#include "mx/core/ScorePartwise.h" -#include "mx/core/ScoreTimewise.h" - -#include -#include -#include - -namespace mx::core -{ - -inline constexpr std::string_view SupportedMusicXMLVersion = "4.0"; -inline constexpr std::string_view DoctypeValueScorePartwise = - "-//Recordare//DTD MusicXML 4.0 Partwise//EN"; -inline constexpr std::string_view DoctypeValueScoreTimewise = - "-//Recordare//DTD MusicXML 4.0 Timewise//EN"; - -struct ExtraAttribute -{ - std::string name; - std::string value; -}; - -class Document final -{ - public: - using Root = std::variant; - - explicit Document(ScorePartwise score) : root_(std::move(score)) {} - explicit Document(ScoreTimewise score) : root_(std::move(score)) {} - - static Result fromXDoc(const ezxml::XDoc &doc); - - bool isPartwise() const noexcept { return std::holds_alternative(root_); } - bool isTimewise() const noexcept { return std::holds_alternative(root_); } - - const Root &root() const noexcept { return root_; } - - const std::vector &rootNamespaces() const noexcept { return root_namespaces_; } - void preserveRootNamespace(ExtraAttribute attr) { root_namespaces_.push_back(std::move(attr)); } - - void toXDoc(ezxml::XDoc &out) const; - - private: - Root root_; - std::vector root_namespaces_; -}; - -} // namespace mx::core -``` - -The current corert harness calls `makeDocument()->fromXDoc(...)`. Keep that as a compatibility -adapter, not as the model itself: - -```cpp -namespace mx::core -{ - -class DocumentIo final -{ - public: - bool fromXDoc(std::ostream &errors, const ezxml::XDoc &doc) - { - auto parsed = Document::fromXDoc(doc); - if (!parsed) - { - errors << parsed.error().message; - return false; - } - document_ = std::move(parsed.value()); - return true; - } - - void toXDoc(ezxml::XDoc &out) const - { - document_.value().toXDoc(out); - } - - private: - std::optional document_; -}; - -using DocumentPtr = std::unique_ptr; - -inline DocumentPtr makeDocument() -{ - return std::make_unique(); -} - -} // namespace mx::core -``` - -`DocumentIo` may be empty while parsing; `Document` never is. That keeps legacy harness shape from -infecting the product model. - -## Parsing policy - -Use two distinct policies: - -1. **Structure is strict.** Unknown elements, unknown attributes, wrong order, missing required - content, and too many bounded elements are errors. -2. **Numeric import may clamp.** Existing mx behavior and fixup sidecars expect out-of-range numeric - values to be clamped into range. That is allowed because the resulting model is valid. - -Do not silently repair these: - -- unknown enum literals; -- failed string patterns; -- missing required children or attributes; -- choice branches with multiple alternatives; -- newer MusicXML root versions. - -Version gating should happen at document parse before dispatching deep into the model. A document -declaring a newer version should return `unsupportedVersion`, letting the corert harness skip it. - -## Serialization policy - -Serialization should be total for every valid model object. - -Rules: - -- sequence-shaped classes emit in schema order; -- repeated choice vectors emit in vector order; -- optional attributes emit only when present; -- defaulted attributes use effective getters for API reads but preserve absence on write; -- fixed attributes emit their fixed literal only when their presence flag is set; -- root namespace declarations captured during parse are written back on the root; -- XML declaration and doctype normalization remain in the corert normalization layer, not in every - model serializer. - -## Template strategy - -The C++ templates should consume more of the Plates than Go/C did: - -- value templates consume `EnumPlate`, `NumberPlate`, `StringPlate`, and `UnionPlate` as Go/C do; -- complex templates must consume `ComplexPlate.content` for child structure; -- attributes still come from `attributes` / `merged_attributes`; -- schema derivation uses `strategy = inherit` from the C++ config default; -- repeated heterogeneous choices render boxed variants; -- pure sequences render named fields; -- repeated homogeneous elements render private vectors and append APIs. - -This must still be done without adding C++ branches to the generator. If a needed datum is missing, -first ask whether it is a neutral schema fact. If yes, it belongs in the Plates for all targets. If -it is C++ spelling, it belongs in `gen/cpp/config.toml` or `gen/cpp/templates/`. - -Expected C++ target manifest shape: - -```toml -[render] -dir = "templates" -format = ["clang-format", "-i", "{dir}/**/*.{h,cpp}"] - -[[render.type]] -strategies = ["enum-class"] -template = "enum.h.tmpl" -output = "{pascal}.h" - -[[render.type]] -strategies = ["enum-class"] -template = "enum.cpp.tmpl" -output = "{pascal}.cpp" - -[[render.type]] -strategies = ["numeric-wrapper"] -template = "number.h.tmpl" -output = "{pascal}.h" - -[[render.type]] -strategies = ["numeric-wrapper"] -template = "number.cpp.tmpl" -output = "{pascal}.cpp" - -[[render.type]] -strategies = ["string-wrapper"] -template = "string.h.tmpl" -output = "{pascal}.h" - -[[render.type]] -strategies = ["string-wrapper"] -template = "string.cpp.tmpl" -output = "{pascal}.cpp" - -[[render.type]] -strategies = ["tagged-variant"] -template = "union.h.tmpl" -output = "{pascal}.h" - -[[render.type]] -strategies = ["tagged-variant"] -template = "union.cpp.tmpl" -output = "{pascal}.cpp" - -[[render.type]] -strategies = ["value-class", "composite-class", "attrs-class", "flag", "inherit"] -template = "complex.h.tmpl" -output = "{pascal}.h" - -[[render.type]] -strategies = ["value-class", "composite-class", "attrs-class", "flag", "inherit"] -template = "complex.cpp.tmpl" -output = "{pascal}.cpp" - -[[render.once]] -template = "Document.h.tmpl" -output = "Document.h" - -[[render.once]] -template = "Document.cpp.tmpl" -output = "Document.cpp" - -[[render.once]] -template = "runtime.h.tmpl" -output = "Result.h" -``` - -The exact filenames can change, but source/header split should not. - -## Rejected designs - -### Raw public structs - -Rejected. This copies the Go/C harness shape and leaks invariants to every caller. A public -`std::vector` or public `std::optional` for required content lets users construct invalid MusicXML. - -### One `std::vector>` for every complex type - -Rejected as the universal representation. It preserves order but erases sequence structure and makes -simple types like `Pitch` unnecessarily dynamic. Use it only for repeated heterogeneous choices. - -### Nullable child structs - -Rejected. `struct NoteChild { Pitch *pitch; Rest *rest; ... }` has invalid states: none set or many -set. C++ has `std::variant`; use it. - -### A virtual `Element` base class - -Rejected. It adds heap allocation and vtables everywhere, makes ownership harder, and hides schema -facts behind runtime checks. MusicXML's complex graph is a DAG; most members can be by value. - -### `std::shared_ptr` everywhere - -Rejected. Shared ownership should be a rare explicit decision, not the model's default ownership -story. Use values for ordinary members and `std::unique_ptr` only to keep repeated heterogeneous -choice nodes small. - -### Strict validation only as a separate pass - -Rejected. A separate `validate()` pass means invalid documents can exist in memory. The product API -should make validity the normal state and use builders/parsers as temporary staging areas. - -## Implementation order - -1. Add the C++ target templates and render manifest without touching generator Python. -2. Generate value types first: enum, number, string, union; compile a values smoke test. -3. Generate simple complex types with attributes and value content. -4. Generate pure sequences such as `Pitch`. -5. Generate repeated choice content such as `PartwiseMeasure`. -6. Generate `Document` and version-gated root dispatch. -7. Wire corert through the compatibility `DocumentIo` adapter. -8. Mark or fix corpus files that are structurally invalid if strict parsing exposes them. -9. Keep Go and C green to prove the target did not contaminate the generator. - -The hard part is not C++ syntax. The hard part is preserving the architecture: C++ must be just -another target while still being a much better product API than the proof targets. diff --git a/docs/ai/design/mx-core-opus-4.7.md b/docs/ai/design/mx-core-opus-4.7.md deleted file mode 100644 index 44655c28f..000000000 --- a/docs/ai/design/mx-core-opus-4.7.md +++ /dev/null @@ -1,1208 +0,0 @@ -# mx::core: the C++ generated typed model - -Status: design proposal. The C++ target's `gen/cpp/config.toml` exists; its `templates/` directory -does not. This document specifies the rendered C++ shapes the templates must produce — every -construct chosen with a reason a reviewer can argue with. It is a sibling to `plates.md` and -`generator-agnosticism.md`: those documents describe the engine; this one describes the artifact -the engine prints for the C++ edition. - -It is intentionally non-template. Every snippet below is what the **generator emits**, not what a -template looks like; the templates are the renderer's problem and live under -`gen/cpp/templates/` once these shapes are agreed. - -The reader is assumed to have read `plates.md` (the eight shapes, the per-target binding) and -`gen/README.md` (the IR, the resolver, the deps-first DAG, the no-collision invariant). Cross- -references in this document use the IR/Plates vocabulary verbatim. - ---- - -## 1. Decisions in one page - -The design hangs off five non-negotiables. Everything in §3-§13 derives from these. - -1. **The complex-type graph is a DAG with no cycles** (`gen/README.md` §"Two load-bearing - invariants"). No forward declarations. No `shared_ptr`. No heap. **Plain by-value composition - throughout.** A `Note` *contains* its children, it does not point at them. - -2. **The schema's `name -> type` map is 1:1.** Parse dispatch is a static name table; we never - need context-sensitive resolution. Serialization writes a known tag for each member. - -3. **The data classes are dumb structs.** Public fields, aggregate-initializable, no invariants - beyond what `enum class` and the value-type wrappers already give us. Validation lives in the - value-type wrappers (clamp on assignment) and in parse (lenient values, strict names — the - plates round-3 contract). Assigning a value of a typed field is by definition valid; that is - the whole point of generating typed wrappers. - -4. **Document order is preserved by structure, not by a separate sequence number.** Pure-sequence - composites use plain ordered fields. Choice-bearing or repeating content uses - `std::vector>` so order is the array order and "exactly one alternative" is - true by construction. **C++ does NOT copy the Go/C ordered-children-of-pointers encoding** — - that pattern exists because Go and C have no sum type. We do; we use it. (`plates.md` §11 - round 3 is explicit about this.) - -5. **Parse and serialize are free functions over `ezxml::XElement`, not member functions.** The - typed model is one concern (data); XML I/O is another (interop). Keeping them apart lets the - model classes be aggregates and lets a future binding (JSON, MessagePack) reuse the same - classes by writing its own free functions. - -The standard is **C++17** (matches the existing `src/private` codebase: `std::optional`, -`std::variant`, `std::filesystem`). No C++20-only features are required. - ---- - -## 2. File and namespace layout - -One type per file, per the existing `[layout] partition = "per-type"` convention (matches Go's and -C's per-type layout, matches what the existing `src/private/mx/core/` directory was sized for, and -gives a 1:1 mapping between IR types and headers — the smallest possible blast radius for a schema -change). Header and implementation are split: - -``` -src/private/mx/core/ - Decimal.h <- hand-written runtime - Decimal.cpp - Runtime.h <- hand-written: SupportedMusicXMLVersion, parse helpers - Runtime.cpp - AboveBelow.h AboveBelow.cpp <- generated, value: enum - Divisions.h Divisions.cpp <- generated, value: number - FontSize.h FontSize.cpp <- generated, value: union - Empty.h Empty.cpp <- generated, complex: empty (presence-only is a - bool typedef; this is for attrs-only emptys) - AccidentalText.h AccidentalText.cpp <- generated, complex: value-bodied - Note.h Note.cpp <- generated, complex: composite (choice content) - Pitch.h Pitch.cpp <- generated, complex: composite (pure sequence) - Mordent.h Mordent.cpp <- generated, complex: derived (inheritance) - Document.h Document.cpp <- generated, document root + extra-attr struct - ... - sources.cmake <- generated, lists all .cpp files for CMake -``` - -File stem is the type's PascalCase identifier (`Plates.Variant.cased["pascal"]` for the type name) -to match the existing C++ convention in this repo (`XElement.h`, `CoreRoundtripImpl.cpp`). - -All generated symbols live in `namespace mx::core`. The C++ Decimal wrapper, the runtime helpers, -and the `SupportedMusicXMLVersion` constant live in the same namespace; the generator does not -produce a separate `mx::core::detail` (no symbol needs to be hidden — every emitted name is part -of the public API of the typed model). - -```cpp -// AboveBelow.h -#pragma once - -namespace mx::core -{ - -// (doc comment ...) -enum class AboveBelow -{ - Above, - Below, -}; - -} // namespace mx::core -``` - -Headers use `#pragma once` (already the convention in `src/private/mx/ezxml/`). No include guards -of the `MX_..._H_INCLUDED` form: pragma once is universally supported by the compilers this -project targets, and matches what is already in the tree. - -Each header `#include`s only the headers it textually needs. The Plates already compute the per- -file `deps`, so the include list is data-driven and stable. The DAG invariant means cycles are -impossible, which is why this works without forward declarations. - ---- - -## 3. Value: enum class - -One file per enum. The wire-literal table and the parse map are emitted into the `.cpp` so the -header is light. The generator's `[target] variant-scope = "bare"` setting (already in -`gen/cpp/config.toml`) means the variant identifier is the bare PascalCase name, scoped by the -`enum class`. - -```cpp -// AboveBelow.h -#pragma once - -namespace mx::core -{ - -/// The above-below type is used to indicate whether one element appears above -/// or below another element. -enum class AboveBelow -{ - Above, - Below, -}; - -/// Returns the wire literal for `v` (static storage, never null). -const char* toString(AboveBelow v); - -/// Strict parse: returns true and writes `*out` only when `s` exactly matches -/// a wire literal. -bool tryParse(const char* s, AboveBelow* out); - -/// Lenient parse: an unrecognized input falls back to the first variant -/// (mirrors the Go/C contract; the corert harnesses depend on this). -AboveBelow parseAboveBelow(const char* s); - -} // namespace mx::core -``` - -```cpp -// AboveBelow.cpp -#include "mx/core/AboveBelow.h" - -#include - -namespace mx::core -{ - -namespace -{ -constexpr const char* kAboveBelowWire[] = { - "above", - "below", -}; -} // namespace - -const char* toString(AboveBelow v) -{ - const auto i = static_cast(v); - if (i >= sizeof(kAboveBelowWire) / sizeof(kAboveBelowWire[0])) - { - return kAboveBelowWire[0]; - } - return kAboveBelowWire[i]; -} - -bool tryParse(const char* s, AboveBelow* out) -{ - if (s == nullptr) { return false; } - const std::string_view sv{s}; - if (sv == "above") { *out = AboveBelow::Above; return true; } - if (sv == "below") { *out = AboveBelow::Below; return true; } - return false; -} - -AboveBelow parseAboveBelow(const char* s) -{ - AboveBelow v; - if (tryParse(s, &v)) { return v; } - return AboveBelow::Above; -} - -} // namespace mx::core -``` - -**Why a `switch`/`if`-chain rather than a `std::unordered_map`?** The -chain is generated, deterministic, branch-predictor-friendly for the common case, and zero-init -at startup; an unordered_map costs a heap allocation per enum and an unordered hash lookup per -parse. With ~96 enums in MusicXML 4.0 the difference is real and free for the generator to -arrange. The chain is the same shape Go and C use. - -**Why overloaded `toString` and `tryParse` (no name composition like `toStringAboveBelow`)?** ADL -plus C++ overload resolution makes `toString(myEnum)` work without the caller naming the type. Go -and C compose names because they have no overloading. We do; we use it. (Free functions, not -member functions, because `enum class` cannot have member functions.) - -**`parseAboveBelow` is composed.** The lenient-fallback parse cannot be overloaded (no input enum -to dispatch on); composition is forced by the language, which is exactly the Variant.cased -[pascal] composed-with-`parse` rule the plates already implement. - ---- - -## 4. Value: number wrapper - -A `class` wrapping the IR primitive's target type, with clamp-on-construct using the plate's -`NumberPlate.clamp` steps (resolved facets + primitive-implied bounds, computed once in the -plates per round 2 of `plates.md` §11). The inner representation is whatever -`gen/cpp/config.toml` `[types]` maps the IR primitive to. - -`Decimal` is `mx::core`'s own hand-written class (lossless decimal-as-text + a numeric face for -arithmetic; the existing corert harness has trailing-zero rules that depend on the lossless -representation, see `DecimalFields.h`). The generator does not invent it; `[types] decimal = -"Decimal"` says "use the existing class," and `Divisions` below wraps it. - -```cpp -// Divisions.h -#pragma once - -#include "mx/core/Decimal.h" - -namespace mx::core -{ - -/// The divisions type is used to express values in terms of the musical -/// divisions defined by the divisions element. It is preferred that these be -/// integer values both for MIDI interoperability and to avoid roundoff -/// errors. -class Divisions -{ -public: - Divisions() = default; - explicit Divisions(Decimal value); // clamp on assignment - explicit Divisions(double value); // convenience - Decimal getValue() const; - void setValue(Decimal value); // clamp on assignment - bool operator==(const Divisions& other) const; - bool operator!=(const Divisions& other) const; - -private: - Decimal m_value{}; -}; - -const char* toString(const Divisions& v, std::string& buf); // see §14 -bool tryParse(const char* s, Divisions* out); -Divisions parseDivisions(const char* s); - -} // namespace mx::core -``` - -For a number with bounds (`positive-decimal`, `tenths`, `midi-channel` with `[1, 16]`, etc.), the -constructor and setter call a generated `clamp` free helper that runs the plate's clamp steps. -The clamp steps are *data on the plate* per round 2; the template emits one branch per step. - -```cpp -// Tenths.h -#pragma once - -#include "mx/core/Decimal.h" - -namespace mx::core -{ - -class Tenths -{ -public: - Tenths() = default; - explicit Tenths(Decimal value); - Decimal getValue() const; - void setValue(Decimal value); - // ... operator== etc. -private: - Decimal m_value{}; -}; -// (parse / toString / tryParse free functions as above) - -} // namespace mx::core -``` - -```cpp -// MidiChannel.cpp (illustrative: integer with min=1, max=16) -#include "mx/core/MidiChannel.h" - -namespace mx::core -{ - -namespace -{ -int clamp(int v) { if (v < 1) { return 1; } if (v > 16) { return 16; } return v; } -} // namespace - -MidiChannel::MidiChannel(int value) : m_value{clamp(value)} {} -void MidiChannel::setValue(int value) { m_value = clamp(value); } -int MidiChannel::getValue() const { return m_value; } - -} // namespace mx::core -``` - -**Why a class with private storage rather than a `using Tenths = Decimal;`?** Type identity. With -strong typedefs, `void f(Tenths)` cannot be silently called with a `Decimal` or a `Divisions`; -that's the whole point of generating ~25 distinct numeric types from the schema. A `using` alias -gives every numeric type the same C++ identity, which throws away the schema's information at the -language boundary. - -**Why explicit constructors?** Same reason: prevent silent conversions from `int` or `double` -landing in the wrong wrapper. A user who wants to construct a `Divisions{4}` must say so. - -**Why a getter/setter pair, not a public `value` field?** Because the setter clamps. A public -field would let the invariant escape. This is the one place in the generated code where a class -has private state; it earns it. - ---- - -## 5. Value: string wrapper - -A class over `std::string`, optionally enforcing a regex pattern and length bounds. Most string -types in MusicXML are unconstrained tokens, so the wrapper is mostly identity; the few with -patterns (`color`, `time-only`, `comma-separated-text`) get the check on `setValue`. - -```cpp -// Color.h -#pragma once - -#include - -namespace mx::core -{ - -/// The color type indicates the color of an element. Color may be represented -/// as hexadecimal RGB triples or as hexadecimal ARGB tuples [...] -class Color -{ -public: - Color() = default; - explicit Color(std::string value); - const std::string& getValue() const; - void setValue(std::string value); - bool operator==(const Color& other) const; - bool operator!=(const Color& other) const; -private: - std::string m_value; -}; - -void toString(const Color& v, std::string& out); -bool tryParse(const char* s, Color* out); // pattern-checked -Color parseColor(const char* s); // lenient: accepts on parse failure - -} // namespace mx::core -``` - -For the unconstrained string aliases (the majority), the wrapper is the same shape but `tryParse` -always succeeds; the class is generated for type-identity reasons (so `void f(CommaSeparatedText)` -will not silently take a `Color`), not for validation. Identical rationale to numeric wrappers. - ---- - -## 6. Value: union (tagged variant) - -`std::variant` over the member types. This is the C++17 sum type; using anything else here would -be reinvention. The plate already carries `UnionPlateMember.tag` (per round 2 of `plates.md` -§11), so each alternative has a stable, projected, collision-checked discriminator. - -```cpp -// FontSize.h -#pragma once - -#include "mx/core/CssFontSize.h" -#include "mx/core/Decimal.h" - -#include - -namespace mx::core -{ - -/// The font-size can be one of the CSS font sizes or a numeric point size. -class FontSize -{ -public: - enum class Kind { Decimal, CssFontSize }; - - FontSize(); // default: first member, default-constructed - explicit FontSize(Decimal v); - explicit FontSize(CssFontSize v); - - Kind getKind() const; - - // Each accessor is valid only when getKind() returns the matching kind. - // std::get throws std::bad_variant_access on mismatch. - Decimal getDecimal() const; - void setDecimal(Decimal v); - CssFontSize getCssFontSize() const; - void setCssFontSize(CssFontSize v); - - bool operator==(const FontSize& other) const; - bool operator!=(const FontSize& other) const; - -private: - std::variant m_value; -}; - -void toString(const FontSize& v, std::string& out); -bool tryParse(const char* s, FontSize* out); // tries members in schema order -FontSize parseFontSize(const char* s); // lenient: first member absorbs - -} // namespace mx::core -``` - -**Why expose `std::variant` *behind* getter/setter rather than inheriting from it or making -`m_value` public?** Two reasons. First, accessor names reveal the schema's intent ( -`getCssFontSize()` reads at the call site; `std::get(fs.value)` does not). Second, -the `Kind` enum gives the caller a `switch`-friendly discriminator without the `std::holds_ -alternative` boilerplate. The generated code's *interface* is the kind enum + per-member accessors; -the variant is the *implementation*. - -**Why not a hand-rolled tagged union (a struct with a `Kind` and a `union { ... }`)?** Lifetime. -The members can have non-trivial destructors (a `std::string`-backed wrapper). `std::variant` -handles the lifetime; a hand-rolled union would have to do it manually and would be a hand-written -piece of generated code per union, which is reinvention. - -**`tryParse` order matters.** The plates round-2 rule "an open string union member must be last" -keeps `tryParse` from short-circuiting. The C++ template enforces this at render time, identical -to Go/C; otherwise lossy schemas would silently route everything through the open string member. - ---- - -## 7. Complex: empty - -Two sub-cases, both already separated by the plates: - -- **Presence-only** (`presence_only: true`, no attributes): the IR's "the only information is - whether it appears." Emit as `using Empty = bool;` — there is **nothing else worth saying**. - An ordered-children variant alternative whose payload is `Empty` is *visibly* a presence flag. - ```cpp - // Empty.h (a few presence-only types are aliased to bool) - #pragma once - - namespace mx::core - { - - /// Presence-only marker. The element is either present (`true`) or absent. - using Empty = bool; - - } // namespace mx::core - ``` - - An alternative — emitting an empty `class Empty {};` — would compile, but `Empty{}` and `Empty{}` - are indistinguishable at every call site, so the type adds nothing the language doesn't already - give us with `bool`. The `using` is honest about the type's information content. - -- **Attrs-only** (`presence_only: false`, attributes only): a class whose fields are the - attributes (§9 conventions for attribute fields). Same shape as a value-bodied complex type - minus the `value` field. - ---- - -## 8. Complex: value-bodied - -A class whose `value` field is the typed text body and whose other fields are the attributes. -Aggregate; default-constructible; field order = attribute declaration order (then `value` last). - -```cpp -// AccidentalText.h -#pragma once - -#include "mx/core/AccidentalValue.h" -#include "mx/core/Color.h" -#include "mx/core/CommaSeparatedText.h" -#include "mx/core/EnclosureShape.h" -#include "mx/core/FontSize.h" -#include "mx/core/FontStyle.h" -#include "mx/core/FontWeight.h" -#include "mx/core/LeftCenterRight.h" -#include "mx/core/NumberOfLines.h" -#include "mx/core/NumberOrNormal.h" -#include "mx/core/RotationDegrees.h" -#include "mx/core/SmuflAccidentalGlyphName.h" -#include "mx/core/Tenths.h" -#include "mx/core/TextDirection.h" -#include "mx/core/Valign.h" - -#include -#include - -namespace ezxml { class XElement; } - -namespace mx::core -{ - -/// The accidental-text type represents an element with an accidental value -/// and text-formatting attributes. -class AccidentalText -{ -public: - // Optional attributes use std::optional (§13). - std::optional smufl; - std::optional xmlLang; - std::optional xmlSpace; - std::optional justify; - std::optional defaultX; - std::optional defaultY; - std::optional relativeX; - std::optional relativeY; - std::optional fontFamily; - std::optional fontStyle; - std::optional fontSize; - std::optional fontWeight; - std::optional color; - std::optional halign; - std::optional valign; - std::optional underline; - std::optional overline; - std::optional lineThrough; - std::optional rotation; - std::optional letterSpacing; - std::optional lineHeight; - std::optional dir; - std::optional enclosure; - - // Required: by-value, default-constructible. - AccidentalValue value{}; -}; - -bool operator==(const AccidentalText& a, const AccidentalText& b); -bool operator!=(const AccidentalText& a, const AccidentalText& b); - -void parseAccidentalText(const ezxml::XElement& el, AccidentalText& out); -void serializeAccidentalText(const AccidentalText& v, ezxml::XElement& parent, - const std::string& tag); - -} // namespace mx::core -``` - -**Public fields, not getters/setters.** The class has no invariant beyond what each *member*'s -type already enforces (`Tenths` clamps on assignment, `EnclosureShape` is an enum class). -Wrapping a getter around an `std::optional` would add ceremony and zero safety. - -**`std::optional` for optional members.** Standard, value-typed (no allocation), trivially -distinguishable from "present with default value" — the case `defaultX = Tenths{0}` is genuinely -different from `defaultX = std::nullopt`, and the wire format reflects it. Go uses pointers -because Go has no `optional`; C uses a `bool has_x` companion because C has no generics. We -have `std::optional`; we use it. - -**`bool operator==` is generated.** Aggregates are not equality-comparable by default in C++17 -(C++20's `=default` would change this). The generator emits the per-field comparison. The corert -harness does not need it (it diffs at the XML level), but the API shape is dramatically more -useful for everyone else (unit tests, custom user code, hashing later). - -**Required fields are by-value.** No `optional` wrapper, no pointer. The DAG invariant means -this is always safe; default construction gives the type's natural zero (a `Tenths{0}`, an -`AccidentalValue::Sharp`). - ---- - -## 9. Complex: composite (the crucial case) - -Composites are the hard one and the place the C++ design diverges from Go and C. A composite has -attributes plus a content tree (`Resolver.content`, group-spliced). Two flavors: - -**9a. Pure-sequence composite, every member exactly once (or optional once), no choices, no -repeats.** Plain ordered fields. `Pitch` (step, alter?, octave) is the canonical case. - -```cpp -// Pitch.h -#pragma once - -#include "mx/core/Octave.h" -#include "mx/core/Semitones.h" -#include "mx/core/Step.h" - -#include - -namespace ezxml { class XElement; } - -namespace mx::core -{ - -/// Pitch is represented as a combination of the step of the diatonic scale, -/// the chromatic alteration, and the octave. -class Pitch -{ -public: - // No attributes on . - Step step{}; - std::optional alter; - Octave octave{}; -}; - -bool operator==(const Pitch& a, const Pitch& b); -bool operator!=(const Pitch& a, const Pitch& b); - -void parsePitch(const ezxml::XElement& el, Pitch& out); -void serializePitch(const Pitch& v, ezxml::XElement& parent, const std::string& tag); - -} // namespace mx::core -``` - -The field order in the class *is* the document order; serialize walks the fields in declaration -order. Parse loops over child elements and dispatches by tag name onto the matching field (ignoring -order, per round-3's "lenient about structure" rule). - -**9b. Choice-bearing or repeating content.** Attributes are still ordinary fields; **child -elements live in one ordered `std::vector` of a per-type `std::variant`.** - -```cpp -// Note.h -#pragma once - -#include "mx/core/Accidental.h" -#include "mx/core/Beam.h" -#include "mx/core/Color.h" -#include "mx/core/CommaSeparatedText.h" -#include "mx/core/Divisions.h" -#include "mx/core/Empty.h" -#include "mx/core/EmptyPlacement.h" -#include "mx/core/FontSize.h" -#include "mx/core/FontStyle.h" -#include "mx/core/FontWeight.h" -#include "mx/core/FormattedText.h" -#include "mx/core/Grace.h" -#include "mx/core/Instrument.h" -#include "mx/core/Level.h" -#include "mx/core/Lyric.h" -#include "mx/core/Notations.h" -#include "mx/core/NoteType.h" -#include "mx/core/Notehead.h" -#include "mx/core/NoteheadText.h" -#include "mx/core/NonNegativeDecimal.h" -#include "mx/core/Pitch.h" -#include "mx/core/Play.h" -#include "mx/core/PositiveDivisions.h" -#include "mx/core/Rest.h" -#include "mx/core/Stem.h" -#include "mx/core/Tenths.h" -#include "mx/core/Tie.h" -#include "mx/core/TimeModification.h" -#include "mx/core/TimeOnly.h" -#include "mx/core/Unpitched.h" -#include "mx/core/YesNo.h" - -#include -#include -#include -#include - -namespace ezxml { class XElement; } - -namespace mx::core -{ - -/// One child element of : exactly one alternative by construction. -/// Document order is the index in Note::children. -/// -/// The `using` aliases the variant; `std::visit` and the dispatch helpers -/// (§9c) work directly on it. -using NoteChild = std::variant< - Grace, // - Empty, // (presence-only) - Pitch, // - Unpitched, // - Rest, // - Tie, // - Empty, // (presence-only) - PositiveDivisions, // - Instrument, // - FormattedText, // - Level, // - std::string, // - NoteType, // - EmptyPlacement, // - Accidental, // - TimeModification, // - Stem, // - Notehead, // - NoteheadText, // - int, // - Beam, // - Notations, // - Lyric, // - Play // ->; - -/// Notes are the most common type of MusicXML data [...] -class Note -{ -public: - // Attributes — same conventions as §8. - std::optional printLeger; - std::optional dynamics; - std::optional endDynamics; - std::optional attack; - std::optional release; - std::optional timeOnly; - std::optional pizzicato; - std::optional defaultX; - std::optional defaultY; - std::optional relativeX; - std::optional relativeY; - std::optional fontFamily; - std::optional fontStyle; - std::optional fontSize; - std::optional fontWeight; - std::optional color; - std::optional printDot; - std::optional printLyric; - std::optional printObject; - std::optional printSpacing; - std::optional id; - - // Children in document order. Each entry is exactly one alternative by - // construction (it is a std::variant); zero/multiple is unrepresentable. - std::vector children; -}; - -void parseNote(const ezxml::XElement& el, Note& out); -void serializeNote(const Note& v, ezxml::XElement& parent, const std::string& tag); - -} // namespace mx::core -``` - -**Why `std::vector>` and not the Go/C "struct of typed pointers" pattern?** The -Go and C encoding (`struct NoteChild { *Pitch pitch; *Unpitched unpitched; ...; }`) exists because -those languages have no sum type; the pointer field IS the discriminator. The cost is real: -"zero or multiple fields set is undefined" — a contract enforced only by docs. In C++, -`std::variant` makes the same guarantee at compile time. The collision -hazard the Go/C round-3 note flags ("harmony has a child element literally named `kind`, so any -synthetic field can collide") evaporates: there is no synthetic field, and there is no `Kind` -enum to collide with. - -**The variant has duplicate alternatives** (`Empty` appears twice: `` and ``). That is -how `std::variant` is *meant* to handle this — alternatives are positional, not nominal — but the -generator must use **the index, not the type**, to dispatch parse/serialize. The generated -`parseNote` writes `out.children.emplace_back(std::in_place_index<6>, parseEmpty(c))` for `` -to disambiguate the two `Empty`s. This is a known C++ idiom; doing it correctly is one line of -generator logic per duplicated alternative. Worked in the dispatch helper below. - -**The `using ...Child = std::variant<...>` is per type, in the type's own header.** Templates need -no helper file; visitors are written inline against the alias. Naming the alternatives lives in -documentation comments on the variant declaration and in the parse/serialize code, where it is -load-bearing. - -### 9c. Dispatch: parse and serialize for choice content - -The generated parse builds the variant directly from the tag name; the generated serialize -`std::visit`s with one lambda per alternative, indexed (so duplicate types route correctly to -their own tag names). - -```cpp -// Note.cpp (sketch — only the children dispatch shown) -void parseNote(const ezxml::XElement& el, Note& out) -{ - // ... attribute loop (omitted, identical in shape to AccidentalText) ... - - for (auto it = el.begin(); it != el.end(); ++it) - { - const auto child = *it; - const std::string& tag = child->getName(); - if (tag == "grace") - { - Grace v; parseGrace(*child, v); - out.children.emplace_back(std::in_place_index<0>, std::move(v)); - } - else if (tag == "chord") - { - // is presence-only -> Empty (which is bool); presence is true. - out.children.emplace_back(std::in_place_index<1>, true); - } - else if (tag == "pitch") - { - Pitch v; parsePitch(*child, v); - out.children.emplace_back(std::in_place_index<2>, std::move(v)); - } - else if (tag == "cue") - { - // Same Empty type as ; index disambiguates. - out.children.emplace_back(std::in_place_index<6>, true); - } - else if (tag == "voice") - { - out.children.emplace_back(std::in_place_index<11>, child->getValue()); - } - // ... rest of the cases ... - else - { - throw std::runtime_error("unknown element <" + tag + "> in "); - } - } -} - -void serializeNote(const Note& v, ezxml::XElement& parent, const std::string& tag) -{ - auto el = parent.appendChild(tag); - // ... attribute writes (omitted) ... - - for (const auto& c : v.children) - { - switch (c.index()) - { - case 0: serializeGrace(std::get<0>(c), *el, "grace"); break; - case 1: el->appendChild("chord"); break; // presence-only - case 2: serializePitch(std::get<2>(c), *el, "pitch"); break; - // ... down through index 23 ... - case 6: el->appendChild("cue"); break; // presence-only, same Empty type - case 11: { auto* v = el->appendChild("voice"); v->setValue(std::get<11>(c)); break; } - // ... - } - } -} -``` - -**Why a `switch` on `index()` rather than `std::visit`?** Two reasons. (1) The tag name is -*per-alternative*, not per-type, and `std::visit` dispatches on type — duplicate alternatives -(`` and `` both `Empty`) can't be told apart by visit. (2) The `switch` on -`std::variant::index()` is a jump table and is friendlier to the optimizer than the nested- -ternary expansion `std::visit` produces for ~24 alternatives. Both reasons are real; either alone -would be enough. - ---- - -## 10. Complex: derived (real inheritance) - -The `gen/cpp/config.toml` does not yet declare `[target] inheritance` (the C target sets `false`). -The C++ default is `true` — derive types extend their base, exactly mirroring the IR's -`complexContent extension` shape. The plates already expose both `base` and `all_members` so the -choice is config, not template logic. - -```cpp -// Mordent.h -#pragma once - -#include "mx/core/AboveBelow.h" -#include "mx/core/EmptyTrillSound.h" // base class -#include "mx/core/YesNo.h" - -#include - -namespace ezxml { class XElement; } - -namespace mx::core -{ - -/// The mordent type is used for both [...] The long attribute is "no" by -/// default. The approach and departure attributes are used for compound -/// ornaments [...] -class Mordent : public EmptyTrillSound -{ -public: - std::optional mordentLong; // attribute "long" (renamed: "long" is reserved) - std::optional approach; - std::optional departure; -}; - -bool operator==(const Mordent& a, const Mordent& b); -bool operator!=(const Mordent& a, const Mordent& b); - -void parseMordent(const ezxml::XElement& el, Mordent& out); -void serializeMordent(const Mordent& v, ezxml::XElement& parent, const std::string& tag); - -} // namespace mx::core -``` - -**Public inheritance, not private.** This is the schema's `complexContent extension` — a -genuine "is-a" relationship, not implementation reuse. A `Mordent` IS-AN `EmptyTrillSound` and -the API contract honors it: a function taking an `EmptyTrillSound&` reads a `Mordent`'s base -attributes correctly. - -**No virtual functions, no virtual destructor.** These are data classes, not polymorphic ones. -There is exactly one place in the schema (the document root, §11) that needs run-time -discrimination, and that one is handled by `std::variant`, not by virtual dispatch. Nothing else -in mx::core calls a function through a base pointer; the base class exists to share fields and -the IS-A relation, not to dispatch. Adding `virtual` would impose a vtable on every leaf type -(228 of them) for a feature nothing uses. (If a future use case appeared, adding `virtual` to the -base class is a one-edit, base-only change — much smaller blast radius than the inverse.) - -**Slicing is fine here.** Without virtual functions there is no slicing problem: copying a -`Mordent` to an `EmptyTrillSound` simply produces an `EmptyTrillSound` with the right fields, -which is the correct behavior. - -**`mordentLong` for the `long` attribute** is the `[reserved] words = ["long", ...]` rename -landing as a Plates-side identifier choice, surfaced through `rename.attribute.mordent.long = -"mordent-long"` in `gen/cpp/config.toml`. The wire form stays `long`; only the C++ identifier -mangles. This is the existing Plates §6.2 mechanism, used as designed. - -**Parse/serialize delegate to the base.** `parseMordent` calls `parseEmptyTrillSound(el, out)` -first to populate the inherited attributes, then handles the three new ones. `serializeMordent` -calls `serializeEmptyTrillSound`'s body inline (the base's serializer takes a `parent` and a -`tag`, so we cannot reuse it directly without writing the same element twice; the generator -emits the merged attribute list in the derived `.cpp` rather than chaining serialize calls). - ---- - -## 11. The document and its roots - -`Document` is the entry point. Two roots (`score-partwise`, `score-timewise`); exactly one is -present. `std::variant` again. - -```cpp -// Document.h -#pragma once - -#include "mx/core/ScorePartwise.h" -#include "mx/core/ScoreTimewise.h" - -#include -#include -#include - -namespace ezxml { class XDoc; } - -namespace mx::core -{ - -/// An attribute on the document root that is outside the schema; in -/// practice this is the namespace declarations (xmlns, xmlns:xlink). -/// Round-trip parity requires preserving them verbatim. -struct ExtraAttr -{ - std::string key; - std::string value; -}; - -/// A parsed MusicXML document. Exactly one root variant is held. -class Document -{ -public: - using Root = std::variant; - - Root root; - std::vector rootNamespaces; -}; - -/// Parse an ezxml document into the typed model. Throws std::runtime_error -/// on a structural error (no root, unknown root tag, unknown attribute or -/// element name); lenient about values per the round-3 contract. -Document fromXDoc(const ezxml::XDoc& doc); - -/// Serialize the typed model back to an ezxml document. The returned doc -/// is freshly allocated by ezxml::XFactory; ownership is the caller's. -ezxml::XDocPtr toXDoc(const Document& d); - -} // namespace mx::core -``` - -This is the only place in the API where `std::variant` shows up at the document level. The -discriminant is the schema (one root or the other; never both, never neither) so the variant is -the natural type. - -**`fromXDoc` returns by value.** RVO/NRVO and move semantics make this free; a `unique_ptr< -Document>` would be ceremony with no payoff (the caller can put the value in a -`unique_ptr` if they want one). - -**`toXDoc` returns `ezxml::XDocPtr`.** That is ezxml's existing ownership type (a `shared_ptr< -XDoc>`). We do not invent a new ownership shape just because we'd prefer a `unique_ptr`; we use -the layer's contract. - ---- - -## 12. Parse and serialize: the API shape - -Pattern, applied uniformly: - -```cpp -// For every complex type T: -void parse(const ezxml::XElement& el, T& out); // throws on structural error -void serialize(const T& v, ezxml::XElement& parent, const std::string& tag); - -// For every value type V: -void toString(const V& v, std::string& out); // append to `out`; never throws -const char* toString(EnumValue v); // for plain enums; static storage -bool tryParse(const char* s, V* out); // strict; returns false on bad input -V parse(const char* s); // lenient: bad input -> first variant / 0 / clamp -``` - -**Why an out-parameter for parse rather than a return?** Parse is the inner loop of the round- -trip; the generated bodies call it ~24 times in `parseNote` alone. Out-parameter avoids one move -per call and lets the caller `emplace` directly into a `std::vector>` slot. -This is a hot path (see `gen/README.md` "the corert test ... ~1,347 files"); we measured it in -the C/Go ports' equivalents, and the generated body is shaped to match. - -**Why free functions, not member functions?** Because the data classes are aggregates (§3, §8). -Adding `parse` / `serialize` member functions would force the classes to be non-aggregates and -prevent designated-initializer construction. The cost is exactly one symbol per type; the benefit -is keeping the data layer pure. - -**Why `const char*` for parse input rather than `std::string_view`?** Most call sites have a -`std::string` in hand from ezxml (`getValue() const` returns `std::string`); both `const char*` -and `string_view` accept that. `const char*` matches what Go's templated free functions look like -when ported and the existing parse/clamp helpers use it. (`string_view` would be equally fine; it -is a preference, not a load-bearing choice. Lock either one and stay consistent.) - -**`throw std::runtime_error` for structural parse failures.** The corert harness already -`try { ... } catch(std::exception& e) { result.message = e.what(); }`; matching that contract -keeps the harness untouched. The lenient-on-values rule means the throws are rare and limited to -unknown names + missing root, which the harness wants to know about. - ---- - -## 13. Optionality and collections in detail - -| IR cardinality | C++ representation | Why | -|---|---|---| -| required (1) | `T value;` | DAG -> always safe by value | -| optional (0..1)| `std::optional value;` | C++17 native; value-typed; clear engaged/disengaged | -| vector (0..n) | `std::vector values;` | C++17 native; contiguous; supports the choice-vector pattern | - -Special cases: - -- `std::vector>` for the children of choice-bearing composites (§9b). -- `std::variant<...>` for a value-type union (§6). -- Required attributes are still `std::optional` if `corert` parity demands it. The plates - round-3 note is explicit: "required attributes included, because the corert contract is 'write - back exactly what was parsed' and corpus files do omit required attributes." The C++ target - inherits this rule. `Note::id` and other required-but-omitted-in-corpus attributes use - `std::optional`; the API does not pretend they are guaranteed present when the data shows they - are not. This is the same trade-off Go made (`*string` for required attributes); we just have a - better wrapper for it. - ---- - -## 14. Hand-written runtime - -Three files, hand-written, live alongside the generated code: - -### 14a. `Decimal.h` / `Decimal.cpp` - -The lossless decimal class. Already an `mx::core` decision per `gen/cpp/config.toml` `[types] -decimal = "Decimal"`. Sketch of API: - -```cpp -namespace mx::core -{ - -class Decimal -{ -public: - Decimal(); - explicit Decimal(double v); - explicit Decimal(int v); - explicit Decimal(const std::string& s); // parse exact text; lossless - - double getValue() const; // numeric face for arithmetic / clamps - const std::string& getText() const; // wire face: lossless reproduction - - void setValue(double v); // re-renders text using the configured policy - void setText(const std::string& s); - - bool operator==(const Decimal& other) const; - bool operator!=(const Decimal& other) const; - // Arithmetic intentionally absent: a typed model is not a calculator. -}; - -} // namespace mx::core -``` - -The trailing-zero handling that `DecimalFields.h` performs at the corert level is the *test* -side of the same coin: `Decimal` is the *production* side (it preserves the input text), and the -corert normalizer strips trailing zeros from both sides for comparison, exactly as today. - -### 14b. `Runtime.h` / `Runtime.cpp` - -The version constant (per round-3, generated from the schema stem so retargeting cannot leave it -stale) plus the small numeric/string parse helpers the generated code calls. - -```cpp -namespace mx::core -{ - -inline constexpr const char* SupportedMusicXMLVersion = "4.0"; - -bool tryParseInt(const char* s, int* out); -int parseInt(const char* s); // lenient: 0 on failure -bool tryParseDecimal(const char* s, Decimal* out); -Decimal parseDecimal(const char* s); - -} // namespace mx::core -``` - -`SupportedMusicXMLVersion` is **generated** into Runtime.h, despite Runtime.h being hand-written -elsewhere — or, more cleanly, the generator emits a separate `SupportedVersion.h` with the -constant and Runtime.h includes it. (Either works; pick the one that keeps the "regen safe" -invariant: any file the generator might rewrite must contain only generated content.) - -### 14c. The existing `mx/ezxml/` layer is the dependency - -The C++ target builds **on** the existing ezxml DOM (XDoc, XElement, XAttribute). It does not -parse XML itself. The corert harness already does this for the file load/save; the generated code -walks ezxml elements. This matches the Go (etree) and C (libxml2) targets; ezxml is C++'s -counterpart. - ---- - -## 15. CMake integration - -The press's `[render]` manifest emits a `sources.cmake` (the C target already does this) listing -the generated `.cpp` files. The repository `CMakeLists.txt` adds the generated directory and -`include()`s the manifest: - -```cmake -# CMakeLists.txt — sketch of the mx::core target -add_library(mx_core STATIC - src/private/mx/core/Decimal.cpp - src/private/mx/core/Runtime.cpp -) -include(src/private/mx/core/sources.cmake) # appends generated .cpp files -target_sources(mx_core PRIVATE ${MX_CORE_GENERATED_SOURCES}) -target_include_directories(mx_core PUBLIC src/private) -target_link_libraries(mx_core PUBLIC ezxml) -target_compile_features(mx_core PUBLIC cxx_std_17) -``` - -Generated `sources.cmake` writes a single `set(MX_CORE_GENERATED_SOURCES ...)` so the build file -remains hand-managed for everything except the per-type list. Same shape as the C target's -`sources.cmake` template. - ---- - -## 16. What to NOT do (and why) - -These are dead-ends a reviewer might independently propose; the design has rejected each. - -- **`std::shared_ptr` / `std::unique_ptr` everywhere.** The DAG invariant means by-value works, so - heap indirection costs allocation and locality for nothing. Reach for a smart pointer only when - the schema invariant changes — and then the choice is the architecture review of the day, not a - default. -- **Virtual destructors / runtime polymorphism.** Nothing in the schema needs it; it imposes a - vtable on every type for one notional use case. See §10. -- **Hand-rolled tagged unions.** `std::variant` exists. See §6, §9b. -- **A synthetic `Kind` enum on each composite's child variant.** The pointer-struct pattern in - Go/C needs it; the variant pattern does not. Adding one would re-introduce the harmony-`` - collision the variant solution avoids by construction. -- **Member function `parse` / `serialize`.** Breaks aggregate-ness; bloats every class with two - symbols that are inherently I/O. Free functions, see §12. -- **Strong typedef via `using`.** Loses type identity at the language level. Class with private - storage instead, §4-§5. -- **Reflection / Boost.Hana / a registry-of-types.** The generator already has the IR; we do not - need C++ to discover at runtime what we know at build time. Templates emit straight-line C++ - from straight-line plate data. -- **A `Visitor` base class for tree walks.** Users who want a visitor write one with `std::visit` - and a member-pointer table; we do not generate one. Nothing in the corert harness or the public - API needs it, and a generated visitor base would be a wide, shallow interface to a deep tree — - the kind of thing the architect-review skill explicitly flags. -- **Coroutines, ranges, `std::expected`.** All would be defensible in a greenfield 2026 design; - none are reachable from C++17, which the rest of `src/private` is on. Holding the line on the - language baseline keeps blast radius local. - ---- - -## 17. What this design intentionally leaves open - -A short list, with the reason each is deferred: - -- **Header/impl split granularity.** §2 says one type per pair. The plates already support a - per-type file; making one type span "header-only inline" plus "out-of-line cpp" is a future - partition setting (`plates.md` §10 lists this as future work). For now: every type gets a - `.cpp`, even the ones whose parse/serialize is short, so the header dependency is identical for - every consumer. -- **`operator==` for variants whose alternatives are duplicated types.** `std::variant`'s default - `==` works on (index, value) pairs, which is what we want. No generator action required, but - worth a unit test on (e.g.) `NoteChild{std::in_place_index<1>, true}` != - `NoteChild{std::in_place_index<6>, true}` to confirm the index discriminates as expected. -- **`std::hash` / set/map keys.** Not generated. A future need can add it as a one-shape template - edit; nothing depends on it today. -- **Streaming serialize.** `serialize` emits to ezxml's DOM, which then writes to a stream. A - direct streaming serializer (skip the DOM) is possible and may be a follow-up if benchmarks - show the DOM allocation hurts; the current shape is the same as the C++ Reference - implementation has used and is fast enough for the corert workload. -- **The `Decimal` class's exact API.** §14a is a sketch; the precise trailing-zero policy and - whether to surface arithmetic operators is an open call. Whatever lands must round-trip the - decimal-fields corpus tests under `mxtest/import/` byte-for-byte. - ---- - -## 18. Architecture review against `arch-review` - -A quick self-audit against the principles the review skill applies, in priority order: - -**Domain boundaries.** The data layer (mx::core types) is decoupled from the I/O layer (ezxml + -free-function parse/serialize). The hand-written runtime (`Decimal`, version constant, parse -helpers) is the smallest possible *seam* — three files. Adding a JSON serializer is "write -parseFooFromJson / serializeFooToJson," not "rewrite Note." Boundary holds. - -**Simple, deep abstractions.** The interface of every generated type is small (fields + free -parse/serialize). The internal functionality of, say, `NumberPlate` clamping or the variant- -index dispatch is rich. Wide-and-shallow would be a typed model that re-exposes XSD facets at -runtime; we do not. - -**Blast radius.** A schema bump regenerates files; no hand-written code changes. Adding an -attribute to one element changes one `.h`/`.cpp` pair. The biggest hand-written surface is -`Decimal`, which is ~2 files; if we replaced it tomorrow, only the `[types]` mapping moves. - -**Clarity.** `enum class`, `std::optional`, `std::variant`, `std::vector` are exactly what every -C++17 reader expects. Naming follows the existing `src/private` convention (PascalCase types, -camelCase fields, `parseFoo` free functions matching `mxtest/import/`). No surprises. - -The one concession the design makes is the pure-sequence vs choice-bearing split (§9a vs §9b). -That's a real bifurcation in the generated code shape, but it is *visible from the plate* -(`ComplexPlate.content` is enough to choose), it is *generate-by-shape* (one template branch, -not per-element specialization), and it directly serves clarity for the consumer (`Pitch::octave` -reads better than `pitch.children[2]`). It earns its complexity. diff --git a/docs/ai/design/mx-core-plan.md b/docs/ai/design/mx-core-plan.md deleted file mode 100644 index d1a59dffb..000000000 --- a/docs/ai/design/mx-core-plan.md +++ /dev/null @@ -1,393 +0,0 @@ -# mx::core plan: the decision record and definition of done - -Status: APPROVED and IMPLEMENTED (2026-06-11; see §7 for the implementation record and the -recorded deviations). This is the binding plan for the generated C++ `mx::core` target. It was -produced by an owner interview on 2026-06-11 that resolved every open conflict between the two -design proposals, `mx-core-gpt-5.0.md` and `mx-core-opus-4.7.md`. Where this document contradicts -either proposal, **this document wins**. Where it is silent, the proposals remain useful reference -material for rendered-shape sketches (the gpt-5.0 doc for API philosophy, the opus-4.7 doc for -plates mechanics, file layout, and dispatch technique). - -The reader is assumed to know `plates.md`, `generator-agnosticism.md` (both since folded into -`gen/DESIGN.md`), and `gen/README.md`. -Nothing here amends the generator architecture: C++ is a target directory (`gen/cpp/config.toml` -plus `gen/cpp/templates/`), and the cardinal rule holds — no language knowledge enters the -generator's Python. One exception is explicitly scoped in §2.9: projecting the content tree and -named groups onto the Plates is *neutral schema fact* work that serves all targets. - ---- - -## 1. The non-negotiables (owner's cardinal requirements) - - -1. **Valid by construction.** It must be impossible to use the public C++ API to produce a - document that is invalid against the MusicXML 4.0 spec. Invalid states are made - *unrepresentable* by the type system wherever possible — including bespoke structural - representations for individual types where the schema's string-with-pattern view hides - structure (RGB color, dates, comma-separated lists). -2. **Errors in exactly two places, nowhere else.** - - Rejected parse: `parse(...) -> Result` is the error boundary. - - Bounded-cardinality append: `maxOccurs` is enforced; an over-cap append is rejected - (`Result`). MaxOccurs enforcement is mandatory, not a documented near-miss. - No other constructor, setter, or factory returns or throws an error. Construction repairs - deterministically (clamping, normalization) instead of failing. -3. **Modern, safe C++20.** No bare `new`/`delete`, no raw owning pointers, smart pointers where - indirection is needed at all, Meyers-Effective-Modern-C++ discipline. Raise CMake to C++20 - when this target lands. -4. **Optimization priorities, in order: compile time, runtime memory, runtime speed, binary - size.** Consequences: light headers / heavy `.cpp` bodies, no template metaprogramming in - generated code, `std::variant` dispatch by index switch (jump table) rather than `std::visit` - towers, boxed alternatives only where alternative sizes demand it. - -## 2. Decisions - -Each decision below was explicitly approved by the owner during the interview. - -### 2.1 XML layer: pugixml directly; ezxml retires - -Generated code consumes **pugixml** (`pugi::xml_node`, `pugi::xml_document`) directly, the same -way Go consumes etree and C consumes libxml2. ezxml — a virtual-interface wrapper whose every -node access allocates a `shared_ptr` and copies `std::string`s — is retired; its swappability -promise is redundant (target swappability lives in the generator, not inside C++). - -- Promote the vendored pugixml (currently buried at - `src/private/mx/ezxml/src/private/private/pugixml.{hpp,cpp}`, version 1.10) to its own - directory, e.g. `src/private/pugixml/`, and **refresh it to the current upstream release** - (1.15 at time of writing). Keep `pugilicense.txt` beside it. -- Domain classes never read or write streams or files. Free functions map model <-> pugixml DOM; - pugixml owns load/save/encoding (it auto-detects UTF-16/Latin-1, matching what libxml2 does - for the C target). DOCTYPE handling needs the `pugi::parse_doctype` flag. -- The corert harness, `mxtest/import` normalization utilities (sort attributes, strip decimal - zeros), and the Fixer are ported to pugixml. Once no client remains, delete - `src/private/mx/ezxml/`. -- Later (0.7): the core now reaches pugixml through the inline `mx::xml` seam, which keeps this - decision's performance while making the XML library a build-time choice; see `xml-backend.md`. - -### 2.2 Value types: structural unrepresentability first, then clamp - -Priority order for representing a constrained value type: - -1. **Make invalidity unrepresentable structurally.** Bespoke shapes are expected and welcome: - - `color` stores numeric RGB(A) components; serialization formats `#RRGGBB`/`#AARRGGBB`. A - malformed hex string cannot exist. - - `yyyy-mm-dd` stores year/month/day integers, clamped to valid ranges. - - `comma-separated-text` stores `std::vector`. - - Prefix-patterned SMuFL glyph-name tokens (`acc\c+` etc.) store the suffix; the prefix is - structural and emitted by the serializer. - - The full audit of patterned/structured value types in the 4.0 schema is the first task of - the values wave (§4, W1): every `StringPlate` with a pattern gets either a structural - representation or an explicit recorded justification for staying a normalized string. -2. **Clamp/normalize on construction** where a deterministic nearest-valid repair exists. - Numbers clamp using the plates' `clamp` steps (`NumberPlate.clamp` — already projected data), - on construction and on `set`. One policy, one spelling, identical to import. -3. **Closed enums need no validation at all**: enum wrapper types whose only constructors are - the named factories; a `static_cast`-proof invalid value is unrepresentable through the - public API. -4. Unconstrained strings construct directly. - -Default construction is always valid: every type's default is its natural valid zero, recursively -(safe because the complex-type graph is a DAG — by-value composition throughout, no forward -declarations, no shared ownership). - -Mechanisms for bespoke shapes, both pre-existing: manifest `[[render.type]] types = ["..."]` -bespoke-template rows (precedent: Go's `yyyy_mm_dd.go.tmpl`), and hand-written runtime files for -schema-independent types (`Decimal`), referenced via `[types]` in `gen/cpp/config.toml`. - -### 2.3 Composites model the schema grammar exactly - -A flat ordered child list (the Go/C encoding) is rejected for C++: it can represent a `` -with two ``es, which violates §1.1. The generated shape follows the resolved content tree: - -| content shape | C++ representation | -|---|---| -| pure sequence | named fields in schema order; serializer walks declaration order, so wrong order is unrepresentable | -| one-shot choice | a `std::variant` member — exactly-one by construction | -| named schema group (`full-note`, `music-data`, ...) | a generated shared type (class or variant alias) reused by every referencing composite | -| repeated choice (e.g. measure music-data) | private `std::vector` — any order/count of alternatives is schema-valid there, so the vector represents nothing invalid | -| repeated homogeneous element | private `std::vector`, read via `std::span` | -| bounded repeat (finite maxOccurs > 1, e.g. `beam` <= 8) | private vector + `add...(T) -> Result` that rejects an over-cap append | -| presence-only element | a flag (bool field in a sequence; an `Empty` marker type as a variant alternative) | -| optional anything | `std::optional` | -| derived complex types | non-polymorphic public inheritance (`strategy = inherit`); no virtual anything | - -Variant alternatives are by-value by default; box with `std::unique_ptr` **only** where a repeated -heterogeneous choice has large alternatives (measure music-data is the known case — a by-value -`variant` would size every slot at the largest alternative). Which groups box is target -data (`gen/cpp/config.toml` `[vars]`), not template hardcode. Duplicate variant alternatives -(`` and `` both presence-only) dispatch by `std::in_place_index` / -`switch (v.index())` — index, never type. - -### 2.4 Import policy: strict names and structure, lenient values, configured repairs - -Parsing (the only door for external data) applies: - -- **Strict on names**: unknown element or attribute -> error. With version gating, an unknown - name is a generator gap or garbage input, never data to preserve. -- **Strict on structure**: wrong sequence order, duplicate choice alternatives, missing required - children, over-cap repeats -> error. Corpus evidence (§3) shows this costs zero files. -- **Lenient on values** (the owner classes these with clamping, not with structure): - - numbers: lexically-strict parse, then range-clamp (the established fixup-sidecar policy); - - enums: an unknown literal falls back to the first variant; - - patterned strings: normalize into the structural representation's nearest valid value. - Every repair still yields a valid model object — leniency never admits an invalid document. -- **Missing required attributes**: inject a configured default. Config data, not generator - logic, e.g. in `gen/cpp/config.toml`: `lyric-language`'s `xml:lang` defaults to `"und"` (BCP 47 - "undetermined" — the spec-correct unknown-language code). A missing required attribute with - *no* configured default is a parse error. The configured-defaults table is the lever if the - corpus or real-world files ever surface more cases. -- **Version gating**: a root declaring a version newer than the generated - `SupportedMusicXMLVersion` (`"4.0"`, emitted from `Plates.schema_version`) is rejected with - `unsupportedVersion`; the harness counts it as a skip. - -The user-facing construction API stays strict in the §1.1 sense regardless: leniency is the -parser's door only, and even the parser's repairs produce only valid objects (the two-door -policy). - -### 2.5 Errors: one boundary type, internal throw - -```cpp -namespace mx::core -{ -enum class ErrorCode -{ - unknownElement, unknownAttribute, missingRequiredElement, missingRequiredAttribute, - wrongElementOrder, tooManyElements, invalidDocument, unsupportedVersion, -}; - -struct Error -{ - ErrorCode code; - std::string path; // e.g. /score-partwise/part[1]/measure[3]/note[2] - std::string message; -}; - -template class Result; // small hand-written expected-like; C++20 has no std::expected - -Result parse(const pugi::xml_document& doc); -void serialize(const Document& d, pugi::xml_document& out); -} -``` - -Internally, generated per-type parse functions **throw** a `ParseError` carrying path context; -the document-level `parse` catches and converts to `Result`. This keeps ~24-arm dispatch bodies -free of per-child propagation boilerplate while the public boundary stays explicit and -exception-free. `serialize` is total for every model object (§1.1 guarantees the object is -valid). Per-type `parse...`/`serialize...` free functions exist (testability) but the documented -public contract is the Document-level pair. - -### 2.6 API spelling and layout - -- Accessors: `step()` / `setStep(Step)` (modern accessor style — owner's pick). Private members - `m_step` (`m_` prefix; chosen so the reserved-word *suffix* mangling can never collide with a - member-suffix convention). -- Types PascalCase, one type per `PascalCase.h`/`.cpp` pair under `src/private/mx/core/generated/` - (committed, like Go/C output), plus a generated `sources.cmake` (C-target precedent). -- `namespace mx::core`; `#pragma once`; headers include only what they textually need (plates - `deps` drive the include list); doc comments from the schema. -- Hand-written runtime beside the generated code: `Decimal.{h,cpp}` (lossless decimal: text face - for round-trip fidelity, numeric face for clamping), `Result.h`/`Error.h`, small parse/format - helpers (`std::from_chars`-based, locale-free). Generated-only files carry the generated - banner; hand-written files never contain generated content (regen-safe split: the version - constant lands in its own generated header). -- Repo `CMakeLists.txt` gains an `mx_core` static library (C++20) that includes `sources.cmake`; - corert links it. clang-format runs as the manifest's `[render] format` hook. - -### 2.7 Corpus actions (evidence in §3) - -- **Fix the 5 repo-authored synthetic fixtures** missing required `xlink:href` - (`data/synthetic/{link.3.0,opus.3.0,part-link.4.0,group-link.4.0,instrument-link.4.0}.xml`) — - they exist to exercise those elements; marking them invalid would erase that coverage. -- **Add `.fixup.xml` sidecars** for the 4 real-world files missing `xml:lang` on - `lyric-language`, using a new fixup operation: **add-attribute-to-expected** (the C++ importer - injects `xml:lang="und"`, so the expected document must gain it too). The Go/C Fixers must - skip operations they don't implement so their suites stay green untouched. -- **No `.invalid` markers needed**; no files leave the corpus. - -### 2.8 Go and C are anemic, and documented as such - -The Go and C targets are **non-validating round-trip harness bindings**: they enforce neither -sequence grammar nor choice exclusivity, and their APIs can construct invalid documents (a note -with two pitches is representable). The owner's position, recorded verbatim in spirit: an API -that can construct an invalid document is unacceptable for any product surface; these targets' -only legitimate role is keeping the generator honest about language-agnosticism, and their -current shape is tolerated only in that role. - -**Follow-up (approved, scheduled after C++ lands):** retrofit Go and C to the same -grammar-enforcing contract, reusing the §2.9 plates extension. Until then AGENTS.md and the -target READMEs must carry the anemic-API warning. - -### 2.9 The one generator change: project content tree and groups onto the Plates - -The Plates currently splice groups away and the Go/C templates read only the flat member list. -C++ templates need the resolved content tree (sequence/choice nesting) and named-group identity -(`full-note` as a shared type). Exposing these is **neutral schema fact** projection — the same -test the sounds fold and the clamp steps passed — so extending `gen/plates/` (and, if needed, -`gen/ir/resolve.py`) is legitimate Python work under the cardinal rule. It must be: - -- consumed via plate fields any target could read (the JSON Schema target is the forcing - function: it already wants `content`); -- covered by `gen/tests/test_plates.py`; -- invisible to Go/C output (regen produces a zero diff for them — part of Gate 5). - -No C++ branch enters Python. `python3 -m gen plates --check` remains the CI gate. - ---- - -## 3. Corpus evidence (measured 2026-06-11, 829 eligible files) - -These facts drove §2.4 and §2.7; re-verify them if the corpus changes. - -- **Missing required attributes: 9 files, 4 patterns, zero caused by 4.0 breaking changes.** - `lyric-language@xml:lang` (4 real-world files; required since 3.0) and `xlink:href` on - `link`/`opus`/`part-link` (5 synthetic fixtures; required since 3.0 / since the element - existed). -- **Structural violations: zero.** Full xmllint XSD validation of all 829 files found not one - wrong-order, unexpected-element, or missing-required-child error. Strict structural parsing - costs nothing. -- **Value violations: 10 files, all already carrying fixup sidecars.** Mostly numeric range - (`sound@dynamics="-1.11"`, `midi-channel 0`, `duration 0` — clamp territory); three files hold - non-numeric garbage (`display-step` of `=`, `accordion-middle` of ``/`test`, empty - `fret`/`string`) — covered by the enum-first-variant / normalization leniency. -- Expected corert C++ counts: **829 pass / 0 fail / 0 skip** (nothing in the corpus declares a - version newer than 4.0). Go/C stay at **777 / 0 / 52**. - ---- - -## 4. Implementation waves - -All template/config work; each wave ends at a named gate and a commit. Generated output is -committed at every wave. Go/C suites must be green at every wave boundary (agnosticism -regression). - -- **W0 — foundations.** Promote + refresh pugixml; hand-written runtime (`Decimal`, `Result`, - `Error`, parse helpers); CMake `mx_core` skeleton + C++20 bump; the §2.9 plates extension with - tests; fix the 5 synthetic fixtures. - *Gate: plates `--check` green, `test_plates`/`test_agnosticism` green, Go/C regen zero-diff - and suites green.* -- **W1 — value types.** Audit every patterned/structured value type and record its - representation (§2.2.1); templates for enum/number/string/union + bespoke rows (`color`, - `yyyy-mm-dd`, `comma-separated-text`, SMuFL names, ...); values smoke test (C-target - precedent). - *Gate: all value types emitted, compile, smoke test green, Gate-4 clamp/factory unit tests - for representative types.* -- **W2 — complex types.** Templates for attrs/value-bodied/sequence/choice/group/derived shapes; - shape unit tests (sequence order unrepresentability, variant exclusivity, maxOccurs - rejection, default-construction validity). - *Gate: full `mx_core` compiles; shape unit tests green; compile time recorded.* -- **W3 — document + corert.** `Document`, version gating, root-namespace preservation, generated - DOCTYPE/version constants; port harness + `mxtest/import` + Fixer to pugixml and the new API; - add the add-attribute fixup op (+ Go/C fixers skip unknown ops); the 4 `xml:lang` sidecars. - *Gate: Gates 1, 2, 3 green (see §5).* -- **W4 — adversarial audit + cleanup.** Run Gate 6; fix findings; delete ezxml and the legacy - harness pieces; AGENTS.md updates; record final compile-time numbers. - *Gate: all six gates green; ezxml gone; docs current.* - ---- - -## 5. Definition of done: the six gates - -corert green alone is **not** done — it proves round-tripping of valid files, not the cardinal -properties (rejecting invalid input; invalid construction being unrepresentable). - -1. **corert C++ green with pinned counts.** 829/0/0 (pass/fail/skip); count drift is failure - even with zero "fails". Go/C stay 777/0/52. -2. **Emitted documents are schema-valid — permanent CI gate.** Every parsed corpus document is - serialized and the *output* xmllint-validated against the 4.0 XSD. Even the 10 - value-violating inputs must emit valid XML (proves the clamp policy end-to-end). This is the - cardinal rule's mechanical proof and it stays in CI, not one-time. -3. **Rejection suite.** Hand-authored invalid documents that parse must refuse — duplicate - ``, `` before ``, unknown element/attribute, missing required child, - root version 5.0 — each asserting the right `ErrorCode` and a meaningful path. -4. **Construction-safety tests.** Runtime: clamping on construct/set, maxOccurs append - rejection, default-construct-every-type-then-serialize-then-xmllint (mechanically - generatable). Compile-time: must-NOT-compile probes (e.g. constructing an enum wrapper from a - raw int) verified by a script asserting compilation fails. -5. **Generator hygiene.** `plates --check` green; `test_agnosticism` green; regen-then- - `git diff --exit-code` (fresh, deterministic, committed output — including zero Go/C diff); - clang-format hook applied. -6. **Adversarial subagent audit.** At every "done" claim (each wave end and before final merge), - a **fresh-context** subagent receives only the MusicXML 4.0 XSD, the generated public - headers, and the instruction: *construct a schema-invalid document using only the public API - — no casts, no UB, no reaching into privates.* It writes and compiles real programs; - serialized output is xmllint-validated; any invalid output is a bug against the API design. - Mirror direction: the subagent crafts invalid input XML it believes parse should reject; - anything that parses but whose re-serialization xmllint-fails is a leniency hole (findings - triaged against the documented clamp policy — designed repairs are expected to pass). Fresh - context is the point: the auditor must not inherit the implementing session's assumptions. - -Compile time (priority #1) is measured and recorded at each wave boundary rather than -hard-gated, so regressions are visible without inventing an arbitrary budget. - ---- - -## 6. Follow-ups (recorded so they don't evaporate) - -1. **Retrofit Go and C to the grammar-enforcing contract** (§2.8), reusing the §2.9 plates - extension: Go gets real sequence fields + sum-type-style choices, C gets tagged unions. - Their corert suites then test the strict contract in three languages. -2. **Decimal contract finalization**: exact trailing-zero/precision policy must round-trip the - decimal-fields corpus byte-for-byte under the existing normalization; revisit once W3 data is - in hand. -3. **Streaming serializer** (skip the DOM) only if benchmarks ever show DOM allocation matters — - not before. - -## 7. Implementation record (added at completion, 2026-06-11) - -All four waves landed and all six gates are green. What an implementer (or auditor) should -know beyond the decisions above: - -- **Corpus actions (§2.7) were already satisfied.** The W0 re-verification (per §3's "re-verify - if the corpus changes") found the 5 synthetic `xlink:href` fixtures and the 4 `lyric-language` - `xml:lang` files already fixed in `data/`; exactly the 10 fixup-sidecar value-violation files - fail xmllint, all clamp-policy territory. No new sidecars and no add-attribute fixup - operation were needed (the C/Go fixers already skip unknown `` type flavors). The - `[import] attribute-defaults` mechanism exists regardless (`lyric-language`'s `xml:lang` - defaults to `"und"`). -- **The §2.9 projection as built**: `Resolver` gained the field view (trivial single-element - groups splice inline; structural groups stay referenced; choice-shaped groups like - `music-data` collapse so reference sites absorb the occurrence); the Plates gained - `ComplexPlate.fields`, `Plates.groups` (group-class / choice-class plates, including - hoisted `{owner}-choice` / `{owner}-group` types renameable via `[rename.type]`), - first-name dispatch sets, nullability, `default_alt` (a nullable choice's natural zero is - its empty alternative, so a default `` is valid), `is_base`, and `structural_deps`. - Go/C/schema output stayed byte-identical throughout (Gate 5 verified at every wave). -- **Boxing knob**: §2.3 suggested `[vars]` for naming which choices box; as built it is a - manifest `[[render.type]] types = ["music-data-choice"]` row selecting the boxed-choice - templates -- still config data, not template hardcode, and it reuses the existing bespoke-row - mechanism instead of inventing a vars convention. -- **Natural-zero refinement**: required opaque-string attributes default to `"P1"` -- the IR - canonicalizes `xs:ID`/`xs:IDREF` to `token`, and the empty string is not a valid ID, so the - default-constructed Document could not otherwise pass Gate 2's xmllint check. -- **Gate 4's xmllint clause, as built**: only the two roots are globally declared in the - schema, so non-root fragments cannot be xmllint-validated in isolation. The mechanical - probe (`roundTripDefaults`, generated) instead serializes every type's natural zero and - re-parses it with the strict grammar; the default Document is xmllint-validated with the - corpus outputs in `make validate-cpp`. -- **Wire-fidelity text faces**: `Decimal`, the comma-separated list types, `time-only`, and - `ending-number` preserve the exact parsed spelling and re-emit it; numeric/structural - mutation drops the face and serializes canonically. This is what keeps corert exact-text - comparisons green without weakening the structural representations. -- **Compile time (priority #1) record**: clean `mx_core`, GCC 13, `-j8` Debug -- W1 - (values only) 14.2s wall / 44s user; W2-W4 (full model, 905+ generated files) ~1m40s wall - / ~5m35s user. -- **Gate commands**: `make test-core-dev` (Gate 1, pinned 829/0/0), `make validate-cpp` - (Gate 2, permanent), `make test-cpp-unit` (Gates 3 and 4 runtime), `make probe-cpp` - (Gate 4 compile-time), `make test-gen` + plates `--check` + regen-zero-diff (Gate 5). -- **Gate 6 (adversarial audit) found and the implementation closed** a systemic lexical hole: - every `xs:ID`/`xs:IDREF`/`xs:NMTOKEN` attribute had been a raw `std::string` setter that - accepted spaces, the empty string, and leading digits and wrote them back verbatim. The IR - had collapsed `xs:ID`/`xs:IDREF` to `token`, hiding the distinction. The fix preserves - `id`/`idref` as distinct IR primitives (a neutral schema fact, parallel to the already- - distinct `nmtoken`), adds the neutral `PlateRef.name_token` flag, and gives the C++ target - hand-written repairing wrappers (`Token` = NCName for id/idref, `NameToken` = the wider - NMTOKEN). A bare string literal can no longer reach a `setID` (the converting constructor is - explicit -- probe 7); parse repairs malformed identity tokens as a designed value-leniency - repair. Go/C/schema map the new primitives to their existing string spellings, so their - committed output is byte-identical. -- **Known limitation (referential integrity, out of scope by §1.1's "wherever possible").** - Two intrinsically valid `Token`s with the same value still serialize duplicate `xs:ID`s, and - a dangling `xs:IDREF` is unconstrained -- both are document-graph properties no per-object - type can enforce locally, and the audit confirmed `xmllint --schema` does not even check - IDREF existence. Enforcing them would require a document-level validation pass, which the - design rejects (the two-error-doors rule). They are the standard XSD-validity-beyond-the- - type-system carve-out, documented here and in AGENTS.md rather than papered over. diff --git a/docs/ai/design/mx-impl-port-brief.md b/docs/ai/design/mx-impl-port-brief.md deleted file mode 100644 index eb6e1782a..000000000 --- a/docs/ai/design/mx-impl-port-brief.md +++ /dev/null @@ -1,99 +0,0 @@ -# Phase 2 Kickoff Brief: the `mx::impl` Port - -Status: SUPERSEDED. The design session happened; every §5 question is settled in -`mx-impl-port-plan.md`, the BINDING Phase-2 decision record. This brief remains as the -evidence snapshot it was. Scope authority remains plan §8 as amended by the design doc. - -## 1. Goal and acceptance (fixed by plan §8) - -**Goal:** `mx::api` works again, implemented via `mx::impl` against the new generated -`mx::core`; api changes minimally invasive (test-harness fallout only). - -**Acceptance:** -- preserved `mxtest/api` + `mxtest/impl` suites ALL GREEN; -- `MX_API` default ON; -- `lib`/`dev`/`test`/`examples-run` Makefile targets restored; -- coverage job naturally grows to cover api/impl. - -**Out of scope:** xcode/xcframework/iOS/macOS distribution (Phase 3). - -## 2. Where Phase 1 left the tree - -- Branch `newgen-integration` (all Phase-1 gates green; see plan §7). The new core lives at - `src/private/mx/core/` (runtime) + `src/private/mx/core/generated/` (model); entry points are - `mx::core::parse(const pugi::xml_document&) -> Result` and - `mx::core::serialize(const Document&, pugi::xml_document&)` in `generated/Document.h`. -- The whole product stack is restored in-tree behind `option(MX_API OFF)` in CMakeLists.txt: - the `mx` static library (api + impl + utility, linking `mx_core`), the `mxtest` binary - (api/control/file/impl/import suites + cpul), and the three example programs. It is wired but - has never compiled against the new core. There is no Makefile target for it yet (deliberate: - no advertised target may be broken). -- The old api-import harness files sit in `src/private/mxtest/import/` next to newgen's - `Normalize.{h,cpp}`/`DecimalFields.h`, as rewrite reference. - -## 3. Measured inventory (run on this tree, commit e713bfc's successor) - -- Old-core includes (`#include "mx/core/..."` paths that no longer exist): **46 of 77** impl - files, **4 of 13** api .cpp files, **7 of 8** `mxtest/impl` files — **348 distinct old-core - headers** referenced in total (mostly `mx/core/elements/*.h`, plus `mx/core/Enums.h`). -- ezxml references (the library is deleted): `src/private/mx/api/DocumentManager.cpp`, - `mxtest/api/{ApiTester,PitchDataTest,SlurTest}.cpp`, and the entire old import harness - (`ImportTest*`, `ExpectedFiles*`, `ChangeValues*`, `SortAttributes*`). -- `DocumentManager` public surface (`src/include/mx/api/DocumentManager.h`): `createFromFile`, - `createFromStream`, `createFromScore`, `writeToFile`, `writeToStream`, `getData`, - `destroyDocument` — an int-handle registry around the old `mx::core::Document` shared - pointer. This is the api<->core seam and the natural first rewrite. -- Suites: `mxtest/api` has 30 test .cpp files (plus ScoreData headers); `mxtest/impl` has 8 - files (CurveFunctions, DirectionReader, DirectionWriter, MagicTemplates, MeasureWriter, - PositionFunctions, PrintFunctions, ScoreWriter) — these assert old-core types directly. -- Examples (`mx/examples/`): api-only consumers; no direct core or ezxml use. - -## 4. Old core -> new core: the deltas impl must absorb - -| Old core (what impl was written against) | New core (what exists now) | -|---|---| -| `std::shared_ptr` element graph (`makeNote()`, `NotePtr`) | value-semantics classes; `std::optional` members | -| `setHasFoo(true)` presence flags | `std::optional` presence (set the optional) | -| `Choice` enum + all alternatives co-allocated | `std::variant` with positional Kind/index dispatch | -| order-free child setters | named fields in schema order; serializer walks declaration order | -| `fromXDoc(std::ostream&, XDoc&)` per element, bool return | parse exists ONLY at the document boundary: `parse(...) -> Result` | -| ezxml `XDoc`/`XElement` DOM | pugixml directly | -| invalid values representable; clamping ad hoc on import | clamp-on-construct wrappers; invalid unrepresentable; bounded `add...` returns `Result` | -| required-with-unbounded: default element pre-pushed into the set | `OneOrMore` (a first element always exists, by type) | -| MusicXML 3.0-era element set | MusicXML 4.0 model + version gating on parse | - -Semantic fallout to adjudicate per-test (plan §8): api tests that relied on old-core leniency, -error behavior, or representational slack need deliberate decisions, not mechanical porting. - -## 5. Decisions the design session must settle (the grill agenda) - -1. **Port strategy for impl:** mechanical file-by-file translation of the 77 impl files vs. - reimplementation around the new core's shape (the old reader/writer split may not map; the - new core's variants and schema-order fields remove whole classes of old impl bookkeeping). -2. **The `DocumentManager` rewrite:** keeps its int-handle/singleton contract (minimally - invasive) — but what does it hold now that `Document` is a value type? What happens to - `mx::core` parse failures that the old api silently tolerated (`Result` carries errors; - the old `createFromStream` had no failure channel)? -3. **api surface changes:** plan says "test-harness fallout only." Is the version field - (`ScoreData::musicXmlVersion`) now 4.0? What does `mx::api` emit as the root `version`? -4. **`mxtest/impl` per-file fate:** the 8 files assert old-core types directly; port or retire - each (e.g. MagicTemplatesTest may be meaningless against the new core). -5. **The api-level corpus roundtrip:** rebuild ImportTest on pugixml, preferring newgen's - `Normalize.cpp`; do `SortAttributes`/`ChangeValues`/`ExpectedFiles` (and `data/expected/`) - survive at all? -6. **`mx::utility` (9 headers, 15 impl files reference it):** keep as-is, shrink, or absorb. -7. **Order of work and gating:** which suite comes green first (impl unit vs api vs import)? - When does `MX_API` flip ON in CI (a new CI job, or extend linux-core)? When do the - `lib`/`dev`/`test`/`examples-run` Makefile targets return? -8. **Old-core idioms with no new equivalent:** e.g. impl code that built partially-invalid - intermediate states before fixing them up — the new core forbids those states; the impl - algorithms that depended on them need redesign, and they should be found early (the - `MX_API=ON` error wall is the cheapest census: configure with `-DMX_API=on` and catalog). - -## 6. Suggested first moves (cheap, evidence-producing) - -1. `cmake -S . -B build/api-wall -DMX_API=on && cmake --build build/api-wall` — capture and - classify the error wall (it is the true, compiler-verified inventory of §3). -2. Rewrite `DocumentManager.cpp` first; everything in `mxtest/api` funnels through it. -3. Pick one thin vertical slice (e.g. PitchData: api struct -> impl reader/writer -> one api - test) to validate the chosen port strategy before committing to all 77 files. diff --git a/docs/ai/design/mx-impl-port-plan.md b/docs/ai/design/mx-impl-port-plan.md deleted file mode 100644 index 3efe71350..000000000 --- a/docs/ai/design/mx-impl-port-plan.md +++ /dev/null @@ -1,475 +0,0 @@ -# Phase 2 Design: the `mx::impl` Port - -Status: APPROVED, BINDING. This is the decision record for Phase 2 of the newgen integration -(the `mx::impl` port), produced by studying the tree and interviewing the owner. It settles -every open question in `mx-impl-port-brief.md` §5 and amends plan §8's scope where noted -(owner-approved). Execute Phase 2 from this document. Scope authority for goal/acceptance -remains `newgen-integration-plan.md` §8; where this document says "amended," it overrides. - -## 1. Goal and acceptance - -**Goal:** `mx::api` works again, implemented via `mx::impl` against the new generated -`mx::core`. - -**Acceptance (plan §8, with owner-approved amendments):** - -- preserved `mxtest/api` + `mxtest/impl` suites ALL GREEN (per-file fates in §7); -- the new corpus-wide api roundtrip gate (§8) green with **zero regression vs. the pre-port - baseline**; -- `MX_API` default ON; -- `lib`/`dev`/`test`/`examples-run` Makefile targets restored; examples build AND run; -- CI green tip-to-toe, including the new `linux-api` job and the extended coverage/macos jobs - (§10); -- coverage job naturally grows to cover api/impl. - -**Scope amendments vs. plan §8's "api changes minimally invasive (test-harness fallout -only)":** the owner approved two deliberate public-api changes — the `DocumentManager` -error channel (§3: `Result`-returning signatures, no escaping exceptions) and the version -fields on `ScoreData` (§4). Everything else on the api surface stays put. - -**Out of scope (Phase 3):** xcode/xcframework/iOS/macOS distribution; the SMuFL -glyph-name migration (§12 — recorded there as open questions with evidence). - -## 2. Port strategy: mechanical translation, algorithms preserved - -**The cardinal rule of the port (owner-mandated): the impl algorithms are preserved -exactly.** They are the glue that makes `mx::api` produce the documents users want; the -logic must be the same — only the *shape of `mx::core` access* changes. NoteReader.cpp -stays NoteReader.cpp; the 77-file structure stays; the reader/writer split and the -`Converter` hub stay. - -The idiom translation table (old core → new core): - -| Old-core idiom | New-core translation | -|---|---| -| `#include "mx/core/elements/Foo.h"`, `Enums.h`, `Decimals.h`, `Document.h` | `#include "mx/core/generated/Foo.h"` etc. (per-type headers; aggregates are gone) | -| `core::NotePtr` / `makeNote()` shared-ptr graph | value construction; `core::Note note{};` | -| `el->setHasFoo(true); el->getFoo()->setValue(v)` | `el.setFoo(V{v})` (`std::optional` presence) | -| `attr->hasDefaultX` / `attr->defaultX.getValue()` | `el.defaultX().has_value()` / `el.defaultX()->value()` (attributes are members of the element class; no separate Attributes struct) | -| `Choice` enum + `getChoice()` + co-allocated getters | `std::variant` alternative classes; positional `Kind`/index dispatch (`choice.kind()`, `asFoo()`) | -| `getFooSet()` (vector of shared ptrs) | `foos()` (`std::vector` / `OneOrMore` by value) | -| old enum `core::StepEnum::a` | factory wrapper `core::Step::a()`, compare via `tag()` | -| ad-hoc clamping on import; invalid values representable | clamp-on-construct wrappers; invalid unrepresentable | -| unbounded `addBeam(...)` | bounded `addBeam(...) -> Result` (propagate, see §3 row `createFromScore`) | -| required-with-unbounded: default element pre-pushed | `OneOrMore` (first element exists by type) | -| `fromXDoc`/`toXDoc` per element | parse/serialize exist ONLY at the document boundary (`generated/Document.h`) | - -**Where the old maneuver is unrepresentable** (e.g. code that built a partially-invalid -intermediate state and fixed it up later, or relied on shared-ptr aliasing to mutate at a -distance), restructure as locally as possible — observable input→output behavior must be -identical. - -**The adjudication log (§11) is mandatory.** Any place where old observable behavior -*cannot* be reproduced (the new core forbids the output, clamps a value the old core passed -through, rejects a file the old api accepted, or vice versa) is not a silent judgment call: -log it in §11 with the decision taken, and update api tests only when the new behavior is -deliberately accepted. - -**Why the vertical slice is non-negotiable:** the `MX_API=ON` error wall was measured -(2026-06: 74 translation units fail, every one a `fatal error` on a missing include — -old aggregate/per-element headers and ezxml). Because each TU dies at its first missing -include, the wall is *shallow*: it confirms the file inventory and reveals zero semantic -errors. There is no cheap semantic census; fallout only surfaces after includes and type -spellings are already translated. The slice (work order step 2) is the price-discovery -mechanism for the whole port. - -## 3. The `DocumentManager` rewrite - -The int-handle/singleton/mutex contract survives. The registry holds -`std::shared_ptr` (wrapping the new value type), which preserves the -`getDocument()` escape hatch byte-for-byte (`DocumentManager.h` itself declares the -`DocumentPtr` alias). ezxml is gone; load/save go through pugixml + -`mx::core::parse`/`mx::core::serialize`. - -**Error channel (owner decision): no exceptions escape; `Result`-style signatures; mx::api -owns its error vocabulary** (public headers must not include private `mx::core` headers; -mirroring keeps the layering and lets api errors carry api-level codes core has no business -knowing). - -```cpp -namespace mx::api { - -enum class ResultCode { - ioError, // file open/read/write failure (api-level) - xmlSyntaxError, // pugixml could not parse the bytes (api-level) - unknownElement, unknownAttribute, - missingRequiredElement, missingRequiredAttribute, - wrongElementOrder, tooManyElements, - invalidDocument, unsupportedVersion, // mirrored from core::ErrorCode - badDocumentId, // handle not in the registry (api-level) - internalError, // caught exception; nothing escapes (api-level) -}; - -struct ApiError { ResultCode code; std::string path; std::string message; }; -template class Result; // ok()/value()/error(), same shape as core's - // Result specialization for writes -} -``` - -| Method | New signature | Semantics | -|---|---|---| -| `createFromFile` | `Result` | pugixml load (`ioError`/`xmlSyntaxError`) → `core::parse` (core errors mirrored) → handle | -| `createFromStream` | `Result` | same, minus file I/O | -| `createFromScore` | `Result` | CAN now fail: bounded appends (beam > 8) etc. **Refuse, don't drop**: when a `ScoreData` describes something the new core won't represent, propagate the error; silently dropping user data is worse than refusing | -| `writeToFile` | `Result` | `badDocumentId` replaces the old silent no-op; `ioError` on write failure | -| `writeToStream` | `Result` | `badDocumentId` replaces the old silent no-op | -| `getData` | `Result` | `badDocumentId` replaces the old silent default-`ScoreData` return | -| `destroyDocument` | `void` (unchanged) | idempotent destruction stays a silent no-op — that is a feature | -| `getDocument` | `mx::core::DocumentPtr` (unchanged) | the documented escape hatch; tests depend on it heavily | -| `getUniqueId` | `int` (unchanged) | | - -Every public method body is wrapped; any `std::exception` from below becomes -`internalError`. `MX_THROW` disappears from the api layer. The mxl (compressed) -special-case message in old `createFromFile` survives as an `xmlSyntaxError` whose message -keeps the issue-66 hint. - -Fallout: every `mxtest/api` caller updates to the new signatures (mechanical), and the -ezxml usage in `ApiTester.cpp`/`PitchDataTest.cpp`/`SlurTest.cpp` is rewritten on pugixml. -The set of parseable files may shift (new core: strict names/structure, lenient values) — -adjudicated per-test in §11. - -## 4. Version semantics - -- **Write side: the root `version` attribute is always `"4.0"`, unconditionally.** mx does - not support writing documents at other versions (that would be a much bigger project); - emitting "3.0" from a 4.0 model was a fiction. `ScoreData::musicXmlVersion` no longer - influences output. Every test expectation asserting `version="3.0"` or an absent - attribute updates as adjudicated fallout. -- **Read side: verbatim preservation.** New field - `std::string ScoreData::declaredMusicXmlVersion` — exactly what the parsed root carried, - empty if absent. `// TODO: verify that we support the declared version` at the api read - seam (core already hard-rejects anything newer than 4.0 with `unsupportedVersion`; the - TODO covers classifying older/malformed declarations). -- The legacy `MusicXmlVersion` enum stays frozen (`unspecified`, `ThreePointZero`) for - source compatibility; the reader still sets `ThreePointZero` iff the declared string is - exactly `"3.0"`. Documented as read-side-informational only. - -## 5. Timewise ↔ partwise - -The old `core::Document::convertContents()` died with the old core; the new core has no -pivot. Port old `ScoreConversions.{h,cpp}` (165 lines at `71fc402`, a shallow-copy regroup -of parts-of-measures ↔ measures-of-parts plus header) into **`mx::impl`** -(`impl/ScoreConversions.{h,cpp}`), translated to value semantics: - -- Its only consumer is `DocumentManager`; the new core's charter (generated model + minimal - hand-written runtime) does not grow hand-written schema-shaped transforms. Promotion to - core would be a later, separate decision. -- The old mutate-and-restore dance in `getData()` (convert stored doc to partwise, read, - convert back) dies: convert into a local `ScorePartwise` copy, read that, stored document - untouched. Same observable behavior (`musicXmlType == "timewise"` round-trips). -- `createFromScore` with `musicXmlType == "timewise"`: build partwise via `ScoreWriter`, - pivot, store a timewise-rooted `Document`. -- The old pivot's shallow copies become real copies under value semantics — strictly safer, - behavior-identical for this use. - -## 6. `mx::utility` fates - -| Header | Fate | -|---|---| -| `Throw.h`, `StringToInt.h`, `Round.h` | keep as-is (live consumers) | -| `Parse.h`, `Enum.h`, `EnumWithString.h`, `JitAllocate.h`, `Lock.h` | delete (dead once `mxtest/import`'s harness goes; zero other refs) | -| `OptionalMembers.h` | rebuild for the new core's shape | - -`OptionalMembers.h` re-keying: the old macros SFINAE-detect public data members -(`attr.defaultX` + `attr.hasDefaultX` + `.getValue()`); the new core exposes member -functions returning `std::optional` (`el.defaultX()`, `el.setDefaultX(...)`). Same generic -application algorithm in `PositionFunctions`/`FontFunctions` etc.; the detection idiom -becomes C++20 `requires`-expressions (`if constexpr (requires { el.setDefaultX(v); })`). -`PositionFunctionsTest` (including the compile-only bogus-class case) pins it. - -General rule (owner): deleting genuinely dead code that follows from these decisions needs -no further sign-off. - -## 7. Test suite fates - -### 7.1 `mxtest/api` — preserved in full; THE acceptance suite - -All 30 test files + ScoreData headers + harness infra (`mxtest/file`, `mxtest/control`) -come back green. This includes the nine **ApiTester file-roundtrip tests** -(ApiChordSimple, ApiK007a, ApiK007c, ApiK009bSlur, ApiK014aFermatas, ApiK015aLayout, -ApiK016aMisc, ApiLy43e, ApiMuAccidentals1): file → `createFromFile` → `getData` → compare -full `ScoreData` against the hand-written expected values → `createFromScore` → write → -re-load → compare again. **Every file tested that way before is still tested that way.** -Changes are limited to: new `DocumentManager` signatures, ezxml→pugixml in the three files -that touch it, and §11-adjudicated expectation updates. - -### 7.2 `mxtest/impl` — port 7, delete 1 - -| File | Fate | -|---|---| -| `MagicTemplatesTest.cpp` | delete (17 lines, includes only, zero tests) | -| other 7 (CurveFunctions, DirectionReader, DirectionWriter, MeasureWriter, PositionFunctions, PrintFunctions, ScoreWriter) | port mechanically: same assertions, new core spellings | - -A ported test that *constructed* a state the new core forbids is adjudicated via §11, not -silently dropped. - -### 7.3 `mxtest/import` — old harness deleted; replaced by corert - -Owner-confirmed: `ImportTest*`, `ExpectedFiles*`, `ChangeValues*`, `SortAttributes*` are -deleted. Measured fact: that harness round-tripped `XDoc → core::Document::fromXDoc → -toXDoc` — the OLD CORE only; `ScoreData`/impl never appear. corert supersedes it strictly -(DOM comparison, numeric equivalence, sidecar fixups, three languages, pinned 829/0/0). -`Normalize.{h,cpp}` + `DecimalFields.h` stay (corert's). **`data/expected/` is deleted** -(its only reader was the dead harness). - -### 7.4 Examples - -`mx/examples/` (api-only consumers) compile and run; `examples-run` restored (§9 step 6). - -## 8. The corpus-wide api roundtrip (new gate) - -The critical test path the old tree never had corpus-wide: **file → api → impl → file**. -Flow per file: load → `createFromFile` → `getData` → `createFromScore` → write → compare -output document to input document, both normalized with corert's pipeline (`Normalize` + -DOM comparison with numeric equivalence). - -`mx::api` is deliberately a subset of MusicXML: files using unsupported features cannot -round-trip with fidelity, by design. The gate therefore uses a **pinned pass-list** -(owner decision), with two modes: - -- **Regression mode (CI):** run exactly the pinned list; every file must pass; zero - tolerance. Files off the list still run as load-survival. -- **Discovery mode (manual make target, never a CI gate):** run the whole corpus, report - which files newly pass/fail — the source of pass-list growth. Additions are deliberate - commits touching the manifest, never automatic. - -**Step 0 — baseline capture (BEFORE any porting; owner requires zero regression vs. the -old library):** - -1. Check out old `master` (`71fc402`) in a scratch worktree; build it with its own build - system (mx-sdk has the toolchain). -2. Run a one-off driver over the corpus: `createFromFile → getData → createFromScore → - writeToFile` — the old library's honest api roundtrip output per file (recording files - it cannot load). -3. Judge each output against its input with the NEW normalize-and-compare machinery (the - same comparator the Phase-2 gate uses — not the old string percent-match). -4. The passing files are the baseline. Commit the manifest - (`src/private/mxtest/api/roundtrip-baseline.txt`) and the capture driver's source. - -**The gate: every baseline file passes the new api roundtrip. Zero regression, hard CI -failure otherwise.** (Old master is a 3.0-era api: files using 3.1/4.0 features mostly land -off-list; the new system passing them later grows the list — progress, not noise.) -Complementary corpus-wide coverage: `ApiLoadSurvivalTest` (already in `mxtest/api`) -continues as the survival check over all files. - -## 9. Order of work and gating - -Constraint: the `mx` library only links when all 77 impl files compile, so staging is -deliberate. Gating rule (as Phase 1): **nothing advertised flips until step 5 is green.** - -- **Step 0 — baseline capture** (§8). First, before porting. -- **Step 1 — the seam:** `mx::api` Result/error types, `DocumentManager` rewrite, - `impl/ScoreConversions` port. These compile standalone against the new core. -- **Step 2 — the vertical slice:** translate `Converter` (the hub) + the PitchData path - (`NoteReader`/`NoteWriter` and dependencies), compiled via a scratch target, to validate - the §2 translation table and surface semantic fallout early. -- **Step 3 — the wave:** mechanically translate the remaining impl files (checklist: - Appendix A). Library compiles at the end of the wave. -- **Step 4 — suites in order:** `mxtest/impl` (unit pins) → `mxtest/api` (acceptance; §11 - log live) → examples build + run. -- **Step 5 — the corpus roundtrip:** build the harness (both modes), run discovery, verify - zero regression vs. the step-0 baseline, commit the pinned list. -- **Step 6 — flip the defaults:** `MX_API` default ON; restore `lib`/`dev`/`test`/ - `examples-run` Makefile targets; add CI `linux-api`; extend coverage and macos (§10). - -### 9.1 Progress (living section; tick Appendix A per file) - -- **Step 0 DONE** (commit `860cce7`): baseline captured from old master 71fc402; the pinned - manifest is `src/private/mxtest/api/roundtrip-baseline.txt` (exactly 1 file passes the - strict comparator — see `src/private/mxtest/api/baseline/README.md`). The shared - normalize/compare machinery lives in `mxtest/corert/Compare.{h,cpp}`; the judge builds - with `-DMX_BASELINE_JUDGE=on`. -- **Step 1 DONE** (commit `550b48c`): `mx/api/Result.h`, `ScoreData.declaredMusicXmlVersion`, - the DocumentManager rewrite (write boundary pins version="4.0" on a serialization-time - copy), `impl/ScoreConversions`, `impl/WriteRefusal.h` (writers throw it for bounded-append - refusal; `createFromScore` catches). -- **Step 2 DONE** (commit `c1ad11b`): Converter + NoteReader/NoteWriter + the - OptionalMembers/Position/Font/Print machinery. Slice findings: translation table held; - Converter tables are now `EnumMap` pair-vectors (wrapper enums have no ordering / no - public Tag ctor); variant choices bridge by nested `Kind` enums; NoteWriter builds - values then installs (`assembleNoteChoice`). -- **Steps 3–6 DONE**: ported files are ticked in Appendix A. The scratch target - `-DMX_IMPL_SLICE=on` (CMakeLists) compiles every ported TU as a static archive — add each - newly ported .cpp to its list. Canonical ported examples to imitate: `Converter.h/.cpp`, - `NoteReader.cpp`, `NoteWriter.cpp`, `PositionFunctions.h` + `utility/OptionalMembers.h`, - `CurveFunctions.h` (generic Slur/Tied handling), `TimeReader.cpp` (old `properties` is the - new core's `attributes`; old `TimeChoice::timeSignature` is `TimeChoice::Kind::group`), - `MetronomeReader.cpp` (old `BeatUnitPerOrNoteRelationNoteChoice` is `MetronomeChoice`, - beatUnitPer=Kind::group, noteRelationNote=Kind::group2). `MX_UNUSED` was rehomed to - `mx/utility/Unused.h`. Verify each TU with - `g++ -std=c++20 -fsyntax-only -I src/private -I src/include `; then build - `cmake -S . -B build/core-dev -DCMAKE_BUILD_TYPE=Debug -DMX_CORE_DEV=on -DMX_IMPL_SLICE=on` - and keep corert green (`build/core-dev/mxtest-core-dev`, 830 cases). `make fmt` - (MX_RUNNING_IN_DOCKER=1 natively) before committing. - - **Step-3 translation notes (from porting sessions):** - - `getAttributes()` is gone — attrs are direct optional members of the element class; pass - the element itself to `getPositionData()`, `getFontData()`, - `setAttributesFromMarkData()`, etc. - - Enum wrappers use factory constructors (`StartStop::stop()`) and `.tag()` comparisons; - there is no public Tag ctor, so `EnumMap` pair-vectors replace the old switch tables in - `Converter.cpp`. - - `DynamicsChoice` is a span of `Empty`/`OtherText` variants (one per dynamic mark), not a - single-value enum; `Dynamics::choice()` returns `std::span`. - - `FermataShape::emptystring` was renamed to `FermataShape::empty()` in the new core. - - `TupletPortion` replaces the old `TupletActual`/`TupletNormal` pair; - `TimeModificationGroup::normalType()` returns `NoteTypeValue` directly (not a wrapped - element ptr); `TimeModificationGroup::normalDot()` is a span. - - Credit writing: `FormattedTextID` words → `CreditChoiceGroup` → - `CreditChoice::group(group)`; reading: `choice().isGroup()` → - `group.choice().isCreditWords()` → `group.choice().asCreditWords().value()`. - - ArticulationsChoice, OrnamentsGroupChoice, TechnicalChoice all use `kind()` + `asXxx()` - dispatch; new 4.0 cases (softAccent, brassBend, flip, smear, open, halfMuted, harmonMute, - golpe, invertedVerticalTurn, haydn) are stubbed with `return false`. - - Mutable optional field pattern: `auto x = outer.field().value_or(T{}); modify(x); - outer.setField(x);` (copy-modify-set, no in-place mutation through optional). - -- **Still to port** (Appendix A unticked): NotationsWriter.cpp (its .h is ported), - DirectionReader/Writer, NoteFunctions, PropertiesWriter, MeasureReader/Writer, - PartReader/Writer, ScoreReader/Writer (.h files for the last two are ported; ScoreWriter - port should DROP the old version-echo switch — the write boundary pins "4.0" now, §4). - Then steps 4-6. - -## 10. CI plan - -- `linux-gen`, `linux-core`, `linux-targets`: unchanged (Makefile recipes build named - targets; `MX_API=ON` default does not drag api code into them). -- **New `linux-api`:** build `mx` + `mxtest` through mx-sdk; run the mxtest suites - (api/impl/file/control); run the corpus roundtrip in regression mode; build and run the - examples. -- **coverage:** the `--coverage` build now includes `mx` (api/impl/utility); mechanics - unchanged, table grows rows — satisfies the "coverage naturally grows" acceptance clause. -- **macos:** extends to build `mx` and RUN the api/impl suites natively with AppleClang - (owner-approved). Same Phase-1 principle: AppleClang fallout is our code, found here, - fixed in Phase 2. -- Discovery mode stays a manual make target, never a CI gate. - -End-of-phase definition of done: the branch tip drives this entire CI matrix green. - -## 11. Adjudication log (behavioral changes) - -Living section, updated during the port. Every entry: what the old behavior was, why it -cannot or should not be reproduced, the decision, and the tests updated. Pre-registered -entries from this design (details in the sections cited): - -| # | Change | Decision | Where | -|---|---|---|---| -| 1 | `createFromFile`/`createFromStream`/`createFromScore` returned `int`, threw on failure | `Result`, no exceptions escape | §3 | -| 2 | `getData`/`writeToFile`/`writeToStream` silently no-opped (or returned default `ScoreData`) on a bad handle | `badDocumentId` errors | §3 | -| 3 | `createFromScore` silently accepted unrepresentable `ScoreData` (e.g. 9 beams) | refuse with error; never drop data silently | §3 | -| 4 | root `version` attribute echoed `ScoreData::musicXmlVersion` (3.0 or omitted) | always `"4.0"` on write | §4 | -| 5 | declared version other than "3.0" was discarded on read | preserved verbatim in `declaredMusicXmlVersion` | §4 | -| 6 | the old `partwiseTimewise` pivot walked a part's measure iterator `currentMeasureIndex` steps with no bounds check: on a ragged score (parts with unequal measure counts) it dereferenced past-the-end — undefined behavior, not a reproducible output | the ported pivot bounds-checks and skips a part that has no measure at that index; behavior identical for every well-formed (equal-measure-count) score | §5, `impl/ScoreConversions.cpp` | -| 7 | `ChordData::miscData` (`vector`) was persisted as XML ProcessingInstructions on the `` element — a non-standard MusicXML extension that the old `mx::core` bolted on outside the XSD | the new generated core is strictly typed from the MusicXML 4.0 XSD and has no ProcessingInstruction mechanism; `miscData` is accepted on write but not emitted and not read back; `SaveProcessingInstructions` test updated to expect empty round-trip | `DirectionWriter.cpp`, `DirectionReader.cpp`, `ChordDataSaveAndLoadTest.cpp` | - -(Subsequent entries appended here as the port discovers them.) - -## 12. Deferred to Phase 3: the SMuFL glyph-name migration (open questions) - -Evidence gathered during this design session (so it is never re-derived): - -- The pre-4.0 workaround lives in the consumer (Komp, `MxNoteWriter::handleMarks`): SMuFL - canonical glyph names (e.g. `dynamicNiente`, accent-staccato/marcato-tenuto/ - marcato-staccato/tenuto-accent glyphs, `brassMuteClosed`) are smuggled as the TEXT - CONTENT of `other-articulation`/`other-technical`/`other-dynamics` via - `MarkData::name` + `MarkType::other*`. -- mx-side breadcrumbs: three `TODO - SMUFLKILL` comments (`impl/Converter.cpp`, - `impl/OrnamentsFunctions.cpp`, `impl/NotationsWriter.cpp`) sit exactly at the - `other-*` read/write sites; `MarkType::customAccentTenuto` round-trips through - `` with the sentinel string `"gjhsldjhglskdjhfg"` - (`MarkData.h:193`); `##misc-data##` footnote smuggling (`NoteData.h:123`) is a separate, - generic channel. -- The proper 4.0 home exists in the new core: the `smufl` attribute (type - `smufl-glyph-name`) on every `other-*` element. - -Open questions for the Phase-3 design session: - -1. Add `std::string MarkData::smufl`? (Sketch: read `smufl` attribute into it; write it - back as the attribute.) -2. Legacy-detection rule for migrating smuggled names: lexical shape vs. a vendored SMuFL - glyph-name lexicon vs. no auto-promotion (Komp migrates on its own side)? -3. Old-reader compatibility: must new-mx output remain readable by old Komp builds (emit - both text and attribute), or attribute-only (clean migration, Komp updates in lockstep)? -4. Fate of `customAccentTenuto`/`getMarkTypeFromCustomString` and the `SMUFLKILL` TODOs. - -Resolution: - -- Exact glyph names live in mark-specific `MarkDataChoice` payloads, not as another common - `MarkData` field. -- A compound dynamic owns its ordered standard and `other-dynamics` components; neighboring marks - are never interpreted as one dynamic. -- Text and `smufl` may coexist. mx does not promote legacy text to a SMuFL name automatically. -- The `customAccentTenuto` compatibility path remains unchanged and can be retired separately. - -## Appendix A: port checklist - -### A.1 `src/private/mx/api/` (4 of 13 .cpp touch core/ezxml) - -- [x] `DocumentManager.cpp` (the seam; §3) -- [x] `EncodingData.cpp` -- [x] `MarkData.cpp` -- [x] `SoundID.cpp` -- [x] public headers: add `mx/api/Result.h` (+`ApiError`), `ScoreData.h` version fields (§4) - -### A.2 `src/private/mx/impl/` translation units - -- [x] AccidentalMarkFunctions.cpp/.h -- [x] ArpeggiateFunctions.cpp/.h -- [x] ArticulationsFunctions.cpp/.h -- [x] Converter.cpp/.h (slice, step 2) -- [x] Cursor.cpp/.h -- [x] CurveFunctions.h -- [x] DirectionReader.cpp/.h -- [x] DirectionWriter.cpp/.h -- [x] DynamicsReader.cpp/.h -- [x] DynamicsWriter.cpp/.h -- [x] EncodingFunctions.cpp/.h -- [x] FermataFunctions.cpp/.h -- [x] FontFunctions.h -- [x] LayoutFunctions.cpp/.h -- [x] LcmGcd.cpp/.h -- [x] LineFunctions.h -- [x] LyricType.cpp/.h -- [x] MarkDataFunctions.h -- [x] MeasureCursor.h -- [x] MeasureReader.cpp/.h -- [x] MeasureWriter.cpp/.h -- [x] MetronomeReader.cpp/.h -- [x] MxVersionDefines.h -- [x] NonArpeggiateFunctions.cpp/.h -- [x] NotationsWriter.cpp/.h -- [x] NoteFunctions.cpp/.h -- [x] NoteReader.cpp/.h (slice, step 2) -- [x] NoteWriter.cpp/.h (slice, step 2) -- [x] OrnamentsFunctions.cpp/.h -- [x] PageTextFunctions.cpp/.h -- [x] PartReader.cpp/.h -- [x] PartWriter.cpp/.h -- [x] PositionFunctions.h -- [x] PrintFunctions.h -- [x] PropertiesWriter.cpp/.h -- [x] ScoreConversions.cpp/.h (NEW, ported from old core; §5) -- [x] ScoreReader.cpp/.h -- [x] ScoreWriter.cpp/.h -- [x] SlideFunctions.cpp/.h -- [x] SpannerFunctions.h -- [x] StaffFunctions.cpp/.h -- [x] TechnicalFunctions.cpp/.h -- [x] TimeReader.cpp/.h -- [x] TupletReader.cpp/.h - -### A.3 `src/private/mx/utility/` (§6) - -- [x] delete `Parse.h`, `Enum.h`, `EnumWithString.h`, `JitAllocate.h`, `Lock.h` -- [x] rebuild `OptionalMembers.h` (C++20 requires) - -### A.4 tests and infra - -- [x] delete `mxtest/import/{ImportTest*,ImportTestImpl*,ExpectedFiles*,ChangeValues*,SortAttributes*}`, `data/expected/` -- [x] `mxtest/impl`: delete MagicTemplatesTest.cpp; port the other 7 -- [x] `mxtest/api`: signature fallout (all files); ezxml→pugixml in ApiTester.cpp, PitchDataTest.cpp, SlurTest.cpp -- [x] baseline capture driver + `roundtrip-baseline.txt` (step 0) -- [x] corpus api roundtrip harness (discovery + regression modes) -- [x] examples compile/run -- [x] CMake: `MX_API` default ON (step 6); Makefile: restore `lib`/`dev`/`test`/`examples-run` -- [x] CI: `linux-api` job; coverage + macos extension diff --git a/docs/ai/design/newgen-integration-plan.md b/docs/ai/design/newgen-integration-plan.md deleted file mode 100644 index bf9b25199..000000000 --- a/docs/ai/design/newgen-integration-plan.md +++ /dev/null @@ -1,278 +0,0 @@ -# newgen Integration Plan - -Status: APPROVED. This is the binding decision record for integrating the newgen work -(the generator pipeline + the generated `mx::core`) with the old codebase on `master`. -It was produced by studying both trees and interviewing the owner; the decisions below -are settled. Execute Phase 1 from this document. - -## 1. Situation - -Three commits define the problem: - -| Ref | What it is | -|---|---| -| `71fc402bd1d89c` | `master` HEAD. Master has NOT moved since the newgen branch-off; this commit IS the merge-base. | -| `b01288cd01629c` | "initial commit" of the newgen effort: deleted 2,073 files (old `mx::core`, `mx::api`, `mx::impl`, ezxml, old tests, xcode, docs, …) and hollowed out README/AGENTS/CMake/Dockerfile/Makefile/CI so codegen work could proceed without distracting context. These deletions must NOT appear in the integrated history. | -| `ba4916f` (tip of `claude/newgen-branch-checkout-b8i3jg`) | The completed newgen work: the language-agnostic generator (`gen/`), the generated C++ `mx::core` (valid-by-construction, pugixml, all six gates green per `docs/ai/design/mx-core-plan.md`), Go/C/JSON-Schema targets, the corert/validate/probe/unit gates, mx-sdk toolchain. | - -The newgen branch is a strict descendant of master, but merging it as-is would carry -`b01288c`'s devastating deletions onto master. This plan replaces that merge with a -**rebuilt branch**: new branch `newgen-integration` created at `71fc402`, followed by a -logical, curated sequence of fresh commits whose trees are assembled from the two -sources (`git checkout -- `). The only deletions in the new history are -the *intentional* ones (§3). No merge to master is performed; the deliverable is a -branch that is ready for a PR. - -## 2. The product stack and the phases - -``` -mx/api -> preserved; changes must be minimally invasive (Phase 2 test-harness fallout only) -mx/impl -> preserved but excluded from build; Phase 2 reimplements it against the new mx::core -mx/core -> the old hand-written core is DELETED and replaced by the generated core -``` - -- **Phase 1 (this plan): combine the branches.** New branch off master containing the - newgen work plus everything preserved from the old codebase. `mx::api`/`mx::impl` are - restored in-tree but excluded from the build behind `option(MX_API ... OFF)`. It is - accepted that they do not compile yet. All newgen gates are green at the tip; the - branch is PR-ready. -- **Phase 2 (follow-up, outline in §8): the `mx::impl` port.** Reimplement impl against - the new core's shape, rewrite the api seam (`DocumentManager`) on pugixml/`mx::core::parse`, - bring the preserved `mxtest/api` + `mxtest/impl` suites back to green, flip `MX_API` - default ON. Opens with its own design doc. -- **Phase 3 (theoretical): distribution.** Reinstate iOS/macOS (Xcode project, - xcframework) with fresh eyes, and consider distribution channels / package managers. - Nothing in Phase 1 or 2 blocks on it. - -## 3. Disposition manifest - -Every path `b01288c` touched, with its settled fate. "old" = take from `71fc402`, -"new" = take from `ba4916f`. - -### Deleted intentionally (the only deletions allowed vs master) - -- `src/private/mx/core/**` (old hand-written core, 1,247 files) — replaced by the - generated core. Its 522-file test suite `src/private/mxtest/core/**` (old) dies with - it (tightly coupled to the old core API, and it took forever to compile). -- `src/private/mx/ezxml/**` (35 files) — retired; the new core consumes pugixml - directly. Outside the old core, ezxml is referenced only by `DocumentManager.cpp` and - old test-harness files, all of which are excluded from the build until Phase 2 - rewrites those seams (§8). Restoring code that references a deleted library is - harmless because it is not compiled. -- `src/private/mxtest/corert/**` (old, 5 files) — superseded by the newgen pugixml port - at the same paths. -- `src/private/mxtest/import/{ImportTest*,ExpectedFiles*,ChangeValues*,SortAttributes*}` - remain *restored-but-excluded* (see below), but old `DecimalFields.h` loses to the - newgen pugixml version at the same path. -- `gen/parse.py`, `gen/ids.py`, `gen/generate.py` (the old generator prototype) — - replaced by the `gen/xsd|ir|plates|press` pipeline. -- `docs/ai/projects/**` — deleted (do not resurrect; restoring it would also duplicate - the 154 synthetic corpus files that were *moved* to `data/synthetic/`). -- `.idea/**` — deleted. -- `xcode/**` (incl. `build_xcframework.sh`) — deleted; deferred to Phase 3. -- `CHANGELOG.md` — stays out; will be dealt with at release time. - -### Restored from `71fc402` (excluded from the build until Phase 2 where noted) - -- `src/include/mx/api/**` (55 public DTO headers; zero core/ezxml references). -- `src/private/mx/api/**` (13 files; only 4 touch core/ezxml — `DocumentManager.cpp` - is the real seam). -- `src/private/mx/impl/**` (77 files; welded to the old core API; Phase 2's subject). -- `src/private/mx/utility/**` (9 headers; used by impl). -- `src/private/mx/examples/**` (3 files; api-only consumers). [excluded] -- `src/private/mxtest/api/**` (43 files) — the Phase-2 acceptance suite. [excluded] -- `src/private/mxtest/impl/**` (8 files) — impl's unit spec; Phase 2 ports or retires - per-file. [excluded] -- `src/private/mxtest/import/` old harness files (ImportTest/ExpectedFiles/ - ChangeValues/SortAttributes) — Phase-2 reference for the api-level corpus roundtrip; - to be rewritten on pugixml. [excluded; coexists with newgen's `Normalize.{h,cpp}` and - `DecimalFields.h` in the same directory] -- `src/private/mxtest/file/**` (10) + `src/private/mxtest/control/**` (1) — harness - infra the api tests depend on. [excluded] -- `gen/quality.py`, `gen/.pylintrc` — the generator quality gates, restored and adapted - (§5.3). - -### Taken from `ba4916f` (the newgen tree, wholesale) - -- `gen/**` (pipeline, targets, tests, committed Go/C/schema generated output). -- `src/private/mx/core/**` (hand-written runtime) + `src/private/mx/core/generated/**` - (the generated model, 910 files) — the directory IS the generated/hand-written boundary. -- `src/private/pugixml/**` (vendored 1.15). -- `src/private/mxtest/{core,corert,import,probe,validate}/**` (the new gates; new - `mxtest/core` is the new core's unit suite, unrelated to the deleted old one). -- `src/private/cpul/**`, `data/**` (incl. `data/synthetic/` moves and sidecars), - `docs/musicxml-*.xsd` and `docs/sounds-*.xml` (the renames stand), - `docs/ai/design/**`, `.gitignore` (union with old entries if any matter). - -### Merged / rewritten by hand - -- `README.md` — start from `git show 71fc402:README.md` and make the **smallest - possible diff**: preserve the original at every sentence and word where preservation - is possible. Update only what is factually wrong now: C++17 → C++20, supported - MusicXML version (new core generates from 4.0), the old build-mode table where - targets no longer exist, and an honest status note that `mx::api` is present but - excluded from the build pending the Phase-2 port. Add a brief generator section. -- `AGENTS.md` — **rewrite the original** (`git show 71fc402:AGENTS.md`), folding in as - much of the newgen AGENTS.md information as possible. The newgen version is heavily - biased toward codegen, but that is not what the project is solely about: the project - is a MusicXML C++ library (`mx::api` is the product surface) that now contains a - generator. Remove the "Ignore git history prior to b01288" instruction — history is - meaningful again. Document the phase state and the `MX_API` flag. -- `CMakeLists.txt`, `Makefile`, `Dockerfile`, `.github/workflows/ci.yaml` — §5. - -## 4. Branch construction - -1. `git branch newgen-integration 71fc402` (the name is not load-bearing). -2. Build each commit in §6 by checking out path sets from the two sources and applying - the hand-merged files. Author fresh commit messages; do not cherry-pick the old - history. -3. **Verification invariant (run before declaring done):** - `git diff --name-status 71fc402 newgen-integration` — every `D` line must appear in - §3's intentional-deletion list. Anything else deleted is a bug. -4. A second useful invariant: `git diff ba4916f newgen-integration` should show only - restorations (api/impl/utility/tests/examples/public headers), the hand-merged - build/docs files, and the gen-quality/lint restoration. -5. Push with `git push -u origin newgen-integration`. Do NOT merge to master; do not - open a PR unless asked. - -**Commit-size rule:** large blasts of generated or vendored code are isolated in their -own commits so that every other commit is reviewable in a differ / on GitHub without -crashing it. Blast commits contain nothing a human needs to read. - -**Green bar:** the TIP must be green (all gates in §7). Intermediate commits in the -sequence are allowed to be transiently unbuildable; do not introduce artificial CMake -churn to make each slice build independently. - -## 5. Build system and CI specifics - -### 5.1 CMake - -Base: the newgen `CMakeLists.txt` (C++20, pugixml, `mx_core`, gated test binaries, -`MX_CORE_DEV` section). Add: - -- `option(MX_API "build the mx::api/mx::impl product library and its tests (Phase 2)" OFF)` - wiring the restored sources (api, impl, utility, examples, mxtest api/impl/file/ - control/old-import) into the `mx` static library + `mxtest` binary, with a comment - marking it Phase-2. Default OFF; it is expected NOT to compile until the port. The - point is that the port has a build home from day one. -- Restore `option(MX_COVERAGE ...)` (`--coverage` flags, non-MSVC guard) from the old - CMakeLists, applied to the new-core targets. -- The whole project is C++20. No dual-standard build. - -### 5.2 Makefile - -Base: the newgen Makefile. Changes: - -- Lean on mx-sdk: add Docker-gated wrappers for the C++ core gates - (`core-dev`, `test-core-dev`, `test-cpp-unit`, `validate-cpp`, `probe-cpp`) following - the existing `gen`/`test-go` pattern, so CI and the documented workflow run everything - through the pinned toolchain. Native in-container/dev-machine invocation keeps working - (the `MX_RUNNING_IN_DOCKER` switch already exists). -- Restore `coverage-core-dev` (old mechanism, new subject: `--coverage` build of - `mx_core` + corert + unit suites, gcovr table) and `check-core-dev` if applicable. -- Restore `gen-quality` and `gen-lint` targets from the old Makefile (§5.3). -- Do NOT restore `lib`/`dev`/`core`/`test`/`test-all`/`examples-run`/`xcode-*` in - Phase 1 — no advertised target may be broken. They return in Phase 2 (Phase 3 for - xcode) when they can actually build. -- `core-dev`/`test-core-dev` keep their newgen meaning (the new core). This reuses the - old names for the same intent: codegen iteration on the core. - -### 5.3 Generator quality gates - -Restore `gen/quality.py` + `gen/.pylintrc` from `71fc402`, adapted: point the analyzed -file set (`GEN_PY`) at the new `gen/` tree, then **re-baseline the floors** — run once, -record the current composite as `GEN_QUALITY_FLOOR` and the current pylint rating as -`GEN_LINT_FLOOR` (floor = current score; ratchet later). Restore the pinned analyzer -venv to the Dockerfile (radon, astroid, pylint, cognitive_complexity — same pins as old -master unless they fail to install, in which case re-pin and say so in the commit). - -### 5.4 Dockerfile (mx-sdk) - -Base: the newgen Dockerfile (Ubuntu 24.04, GCC 14, Go, libxml2-dev, clang-format-18, -ccache). Add back from old master: `gcovr`, the quality venv (§5.3). Add -`libxml2-utils` (xmllint, needed in-container by `validate-cpp` once it is -Docker-gated). - -### 5.5 CI (`.github/workflows/ci.yaml`) - -Rebuilt from scratch (the newgen stub is stale — it still references ezxml). All linux -jobs run through mx-sdk. Reuse the old workflow's mechanics for ccache/build-volume -caching and PR comments. - -| Job | Contents | -|---|---| -| `linux-gen` | fmt-check; `make test-gen`; `plates --check` (all targets); regen-then-`git diff --exit-code` (committed generated output must match the generator); `gen-quality` + `gen-lint`, with the quality-report PR comment from the old workflow | -| `linux-core` | `test-core-dev` (corert **829/0/0 pinned**), `test-cpp-unit`, `validate-cpp`, `probe-cpp` | -| `linux-targets` | `test-go`, `test-c` | -| `coverage` | `coverage-core-dev` over the new core, gcovr table, HTML artifact, PR comment — same mechanism as old master | -| `macos` | native (no Docker): build `mx_core` + run unit and corert suites with AppleClang. The new core has only ever been compiled with GCC 14; AppleClang fallout is found here and **fixed in Phase 1** (it is our code) | - -## 6. Commit sequence - -Content assembled per §3/§4; messages are fresh; blast commits marked **[blast]**. - -1. **`gen:` replace the generator prototype with the pipeline.** Delete - `parse.py`/`ids.py`/`generate.py`; land `gen/` pipeline + target configs/templates + - gen tests; restore+adapt `quality.py`/`.pylintrc` (floors re-baselined); corpus moves - (`data/`, incl. `data/synthetic/`), XSD/sounds renames in `docs/`; - `docs/ai/design/` docs. Excludes committed generated output. -2. **`gen:` generated target output [blast].** `gen/test/go/mx/`, `gen/test/go/vendor/`, - `gen/test/c/mx/`, `gen/schema/out/`. -3. **`core:` vendor pugixml 1.15 [blast].** `src/private/pugixml/`. -4. **`core:` replace mx::core.** Delete old `src/private/mx/core/**` and - `src/private/mx/ezxml/**`; land the hand-written runtime (`src/private/mx/core/*`) - and `src/private/mxtest/{core,corert,import,probe,validate}` (new gates), `cpul`. - (Old `mxtest/core` + old corert deleted here.) -5. **`core:` the generated model [blast].** `src/private/mx/core/generated/` (910 files). -6. **`api:` preserve mx::api/mx::impl behind MX_API.** Restore public headers, api, - impl, utility, examples, mxtest api/impl/file/control/old-import per §3; CMake - `MX_API=OFF` wiring. -7. **`build:` merged CMake/Makefile/Dockerfile.** §5.1–5.4 (whatever of CMake/Makefile - wasn't already needed by commits 4–6 to keep the tip story coherent; transient - unbuildability of intermediate commits is acceptable). -8. **`ci:` rebuilt workflow.** §5.5. -9. **`docs:` README minimal-diff revision; AGENTS.md rewritten from the original;** - this plan rides along if not already present. - -## 7. Phase-1 acceptance gates (the tip must pass ALL) - -1. `git diff --name-status 71fc402 `: every deletion is in §3's intentional list. -2. `make test-core-dev` — corert 829/0/0 (pinned-counts case enforces it). -3. `make test-cpp-unit`, `make validate-cpp`, `make probe-cpp` — green. -4. `make test-gen`; `plates --check` for all four targets; regen all targets then - `git diff --exit-code` (zero drift). -5. `make gen-quality`, `make gen-lint` — at or above the re-baselined floors. -6. `make test-go`, `make test-c` — 777/0/52 each. -7. `make check` (fmt) — clean. -8. CI workflow is syntactically valid and its jobs map 1:1 onto the targets above. -9. README/AGENTS.md done per §3 ("merged / rewritten by hand"). -10. Branch pushed (`git push -u origin newgen-integration`). NO merge to master; NO PR - unless the owner asks. - -## 8. Phase 2 outline (the `mx::impl` port) — scope only, opens with its own design doc - -- **Goal:** `mx::api` works again, implemented via `mx::impl` against the new - `mx::core`; api changes minimally invasive (test-harness fallout only). -- **Acceptance:** preserved `mxtest/api` + `mxtest/impl` suites ALL GREEN; `MX_API` - default ON; `lib`/`dev`/`test`/`examples-run` Makefile targets restored; coverage job - naturally grows to cover api/impl. -- **Known seams (inventoried during planning):** - - `src/private/mx/api/DocumentManager.cpp` — the api↔core boundary; rewritten from - ezxml `XDoc` to `mx::core::parse(const pugi::xml_document&) -> Result` - and the serializer. - - ezxml-referencing files needing rewrite: `DocumentManager.cpp`, - `mxtest/api/{ApiTester,PitchDataTest,SlurTest}.cpp`, and the old `mxtest/import` - harness (rebuild the api-level corpus roundtrip on pugixml; prefer newgen's - `Normalize.cpp`; old `SortAttributes`/`ChangeValues` likely retire). - - Semantic fallout to adjudicate per-test: the new core clamps values on - construction, dispatches choices by variant Kind, enforces schema order - structurally, and returns `Result` from bounded appends — api tests that relied on - old-core leniency or error behavior will need deliberate decisions. - - `mxtest/impl` (8 files) assert old-core types directly: port or retire per-file. -- **Out of scope for Phase 2:** xcode/xcframework/iOS/macOS distribution (Phase 3). - -## 9. Phase 3 (theoretical) - -Reinstate iOS/macOS support with fresh eyes (the old checked-in Xcode project may not -have been working correctly anyway) and consider distribution channels and package -managers. Nothing earlier depends on it; nothing here constrains it. diff --git a/docs/ai/design/release-process.md b/docs/ai/design/release-process.md deleted file mode 100644 index 988bcfaf4..000000000 --- a/docs/ai/design/release-process.md +++ /dev/null @@ -1,438 +0,0 @@ -# Release process design - -Status: IMPLEMENTED. This is the design record for `mx`'s GitHub Actions release system. It was -produced from a deep-research pass on how C++/Swift libraries do continuous delivery on GitHub, -followed by an owner interview, then revised twice: once after a design review found mechanical -flaws in the first draft, and once for the owner requirement that release tags live on `main` -(§13 records each change and why). The workflows in §4 exist and this document is kept in sync -with them; §13's implementation entry records where the build drifted from the design text. - -## 1. Goal - -Ship binary releases of `mx` for multiple package ecosystems from a chosen commit, with a -provable guarantee that what gets published is exactly what was built and validated — never a -silent rebuild from a different tree. Swift (a binary XCFramework consumed via SwiftPM) is the -first release target. Conan is a planned second target; the architecture must not be Swift-only. -Release tags are part of `main`'s history (owner requirement): every published version is -reachable from `main`, so `git log`, `git describe`, and `git tag --merged main` all see it. - -The design deliberately assembles this from GitHub primitives rather than copying a precedent: -the deep-research pass found no real-world project running this exact two-phase pattern (phase 1 -builds a chosen SHA and writes a tag-ready release commit; phase 2's tag push re-verifies that -commit from primary sources and publishes). Most projects do single-phase (tag-triggered build -+ publish, or a `workflow_dispatch` version-bump). The two-phase split exists here to make the -"published bytes == validated bytes" invariant airtight. - -## 2. Human touchpoints - -The entire release is four manual actions. Everything else is automation reacting to events. - -1. **(Optional) Open and merge a PR** for any real work the release needs. → commit `A` on `main`. -2. **Run `release-build`** (a `workflow_dispatch`), passing SHA `A` and the version: - `MAJOR.MINOR.PATCH`, or `MAJOR.MINOR.PATCH-prerelease` for a release candidate (§7). -3. **Review and merge the auto-opened release PR** (branch `release-prep/v`), using - **"Create a merge commit"** — not squash or rebase. §3 explains why the merge method matters; - §5 enforces it. -4. **Push a tag** (`v` followed by that same version) pointing at commit `B` — the first of - the PR's two commits, whose SHA the run summary and the PR body both print alongside the - exact `git tag`/`git push` commands. - -No other human step exists. In particular there is no "click publish on a draft," no manual -checksum edit, and no reviewer-approval gate beyond the tag push itself — pushing the tag *is* -the approval, and the review in step 3 is what the tag pusher is vouching for. - -`release-build` can be triggered three interchangeable ways, since `workflow_dispatch` is one -API event: the GitHub Actions UI ("Run workflow" → `sha` and `version` fields), the `gh` CLI -(`gh workflow run release-build.yml -f sha=<40-hex> -f version=1.4.0`), or the GitHub MCP tool -(`actions_run_trigger`, `method: run_workflow`, `inputs: { sha, version }`). In all three, the -dispatch `ref` selects only *which copy of the workflow YAML runs* (normally `main`); the commit -actually built is the `sha` input, which the job checks out explicitly. - -## 3. The checksum constraint (why `B` exists, and where it sits) - -SwiftPM resolves a version by checking out the *tagged commit* and reading `Package.swift` as it -exists there. For binary-release mode to work for a consumer depending on `from: "1.4.0"`, the -`Package.swift` at the `v1.4.0` commit must already contain the correct -`binaryTarget(url:checksum:)`. The URL is predictable ahead of time — it is a fixed function of -the tag name and a fixed asset filename, which is exactly why the version must be supplied at -build time (§7). The **checksum is not** — it can only be computed after the XCFramework is -built. - -A commit cannot contain the hash of its own future build output; that is a property of content -hashing, not a fixable limitation. Therefore *some* commit must exist between "the SHA the owner -picked" (`A`) and "the SHA that gets tagged" (`B`), carrying the checksum that `A`'s build -produced. - -**The shape that puts `B` on `main` without putting the binary manifest on `main`:** the release -PR that `release-build` opens carries exactly two machine-written commits on branch -`release-prep/v`, based on `A`: - -- **`B`** (parent: `A`) — writes the binary-default release `Package.swift` (§8) and - `release-manifest.json` (§5). This is the commit the tag points at. -- **`R`** (parent: `B`) — restores `Package.swift` to `A`'s copy verbatim - (`git checkout A -- Package.swift`; purely mechanical, no target involvement), keeping - `release-manifest.json` as a visible release record on `main`. - -Merging the PR with a merge commit `M` makes `B` part of `main`'s history — the same way every -commit of every merged feature branch is on `main` — while `main`'s *tree* changes only by the -release-record file. This resolves the conflict that sank both earlier drafts: a consumer's -SwiftPM evaluates `Package.swift` in the consumer's own environment, and there is no way to make -every downstream consumer set an environment variable, so the tagged commit must resolve to the -`binaryTarget` **by default**; meanwhile every tip of `main` must default to a source build for -sibling-checkout development and the non-Apple CI consumers. One file cannot default both ways -at one commit — so the binary default exists at exactly one commit, `B`, and `R` closes it out -before the branch meets `main`. - -Two properties fall out of `B` having parent `A` (rather than being the merge or squash commit, -as in the first draft): - -- *No frozen `main`.* Whatever lands on `main` between `A` and the merge, `B`'s tree is still - exactly "`A` + declared release metadata," so reusing `A`'s build artifacts for `B` stays - sound. Concurrent merges can at most conflict on `release-manifest.json`, which nothing else - touches. (The first draft tagged the merge commit itself, which silently required `main` to be - frozen for the duration of every release.) -- *The PR's net diff is tiny.* GitHub's "Files changed" view (merge-base `A` to head `R`) shows - only `release-manifest.json`; the binary manifest appears in the commits tab at `B`, which the - run summary and PR body link directly for review of the URL/checksum lines. - -**The merge method matters.** Only a true merge commit preserves `B` in `main`'s -ancestry. A squash or rebase merge copies the net diff instead — harmless to `main`'s tree (the -net diff is the record file), but `B` would then be reachable only via the tag, violating the -tags-on-`main` requirement. `release-publish` hard-fails in that case (§5, check 3); recovery is -`git merge -s ours release-prep/v` pushed to `main` — a history-only merge whose tree -change is nil, since `main` already carries the net diff — then re-push the tag. - -**All release-metadata edits from all targets are bundled into the single commit `B`.** If a -future target (Conan) has an analogous "the tagged commit must already carry a computed value" -gotcha, its files go into `B` too — not a second commit or PR. `B` is the one and only place -release metadata is written into a tree. - -Because `B` differs from `A` only in declared release-metadata files (never `src/`), `A`'s build -output is byte-identical to what a build at `B` would produce. The system relies on this: it -does **not** rebuild at `B`; it publishes `A`'s already-built artifacts for `B` (§5). - -## 4. Workflows - -All new, all separate from `ci.yaml`. Names are indicative. - -| File | Trigger | Responsibility | -|---|---|---| -| `release-build.yml` | `workflow_dispatch`, inputs `sha` (required; validated against `^[0-9a-f]{40}$`) and `version` (required; validated as SemVer 2.0 with an optional pre-release suffix and no build metadata, §7) | Checks out `sha` explicitly. Calls one reusable target workflow per release target. Uploads each target's artifacts (`retention-days: 90`). A final prep job collects every target's metadata files, writes `release-manifest.json` (§5), commits `B` then `R` on `release-prep/v` (force-push; the branch is machine-owned), opens the release PR, and prints `B`'s SHA, the tag commands, and the `A...B` compare link in the run summary and the PR body. | -| `_release-target-swift.yml` | `workflow_call` | Builds three slices via CMake + an iOS toolchain file: macOS universal (`arm64`+`x86_64`), iOS device (`arm64`), iOS simulator (`arm64`+`x86_64`). Stitches them with `xcodebuild -create-xcframework`, zips, computes the SHA256 via `swift package compute-checksum`. Outputs: artifact name, checksum, the complete release-mode `Package.swift` content (§8), and the list of paths it is allowed to occupy in `B`. | -| `_release-target-conan.yml` | `workflow_call` | *Future.* Same input/output contract as the Swift target (SHA + version in; artifact + metadata files + allowed-path list out). Not built now; its existence in this table is the proof the abstraction is not Swift-only. | -| `release-publish.yml` | `push: tags: ["v*"]` | Resolves the tag to `B`, reads `release-manifest.json` from `B`'s tree, and re-verifies every claim in it (§5). Hard-fails on any mismatch. Otherwise downloads the pinned run's artifacts and publishes a GitHub Release immediately (not draft), `generate_release_notes: true`, attaching each target's archive and its `.sha256`. | - -No workflow reacts to the release PR's merge; merging only places `B` into `main`'s history, and -all validation happens at tag time. (The first draft had a `release-prep-merged.yml` for this; -it is gone, along with its label machinery.) - -**Implementation notes.** Each target hands its results to the prep job through a companion -artifact, `mx-release--metadata-`: a `tree/` directory holding the exact files to -commit in `B` (for Swift, the interpolated release `Package.swift`) plus `fragment.json` (target -name, artifact name, archive filename, checksum, platforms, allowed paths) — the concrete form -of the §9 contract's outputs (b) and (c). The prep job merges every target's `tree/`, builds -`release-manifest.json` from the fragments, and refuses to commit if anything outside the -declared paths is dirty. The prep job pushes the branch and opens the PR with the workflow's own -`GITHUB_TOKEN`, and GitHub suppresses workflow triggers for events created with that token — so -**the release PR runs no CI**. That is acceptable by design: CI validated `A` on its own PR, -`release-build` built and checksummed `A`, and the release PR's content is machine-written -metadata. Two repository settings are prerequisites: **"Allow GitHub Actions to create and -approve pull requests"** (Settings → Actions → General) must be on, and **"Create a merge -commit"** must be an allowed merge method (§3). - -One bootstrap consequence of the `push: tags` trigger: GitHub runs the workflow YAML as it -exists *at the tagged commit* — i.e. `B`, whose tree is `A` plus metadata. So only SHAs that -already contain `release-publish.yml` are releasable. Acceptable; no escape hatch needed. - -## 5. The SHA-binding invariant - -Phase 2 (`release-publish`) must publish exactly the bytes phase 1 validated. Two design choices -enforce this: - -- **No rebuild.** `release-publish` never compiles anything. It fetches an already-built artifact - by workflow-run ID and publishes it verbatim. This removes rebuild drift as a possible failure - mode entirely. (Build-provenance attestation — `actions/attest-build-provenance`, - SLSA/Sigstore — was evaluated and rejected for this role: the research found no confirmed - mechanism by which it proves phase 2's artifacts equal phase 1's. Artifact download by run ID - is the reliable mechanism.) -- **`B` carries its own provenance, and publish re-derives every claim from primary sources.** - `release-manifest.json`, committed in `B` by the prep job, records: the version, the base SHA - `A`, the `release-build` **run ID** (`github.run_id` is available to a run while it executes, - so the run that builds the artifacts can pin itself into `B`), each target's artifact name and - checksum, and the union of paths the targets declared they may touch. `release-publish` trusts - none of it blindly; at tag time it hard-fails unless **all** of the following hold: - 1. the tag name equals `v` + the manifest's version (so the asset URL baked into - `Package.swift` can never desync from the tag actually pushed); - 2. the tagged commit has exactly one parent and it is the manifest's `A` — which also catches - tagging the wrong commit: `R` fails it (its parent is `B`), as does the merge commit (two - parents), and `A` itself has no manifest at all; - 3. `A` is an ancestor of `main` (releases come from mainline history only) **and `B` is an - ancestor of `main`** — the tags-on-`main` guarantee, which requires the release PR to have - been merged, with a true merge commit, before the tag is pushed (§3 gives the recovery when - it was squashed); - 4. the `A..B` diff touches only the declared paths — additionally clamped by a static ceiling - hardcoded in `release-publish.yml` itself (release metadata paths only; never `src/`, - never `.github/`); - 5. the pinned run exists **in this repository**, is a run of `release-build.yml`, and - completed successfully; - 6. its artifacts download, and a freshly recomputed SHA-256 of each archive equals both the - manifest's checksum and the checksum string inside `B`'s `Package.swift` (for the Swift - target). For a zip, `swift package compute-checksum` *is* the SHA-256 of the file, so plain - `sha256sum` on the publish runner reproduces it. - -`R` and the merge commit are deliberately *not* validated at publish: they affect only `main`'s -tree, which the PR review in step 3 governs. - -Why not query the Actions run history by commit, as the first draft did? Two API facts make that -unsound: for a `pull_request`-triggered run, the Actions API reports `head_sha` as the *PR -branch head*, never the merge commit — so the draft's lookup ("the validating run whose -`head_sha` is the tagged SHA") could never match anything; and a `workflow_dispatch` run cannot -be located by its `sha` *input* at all (inputs are not queryable, and such a run's `head_sha` is -the dispatch ref's head, not the input). Pinning the run ID inside `B` solves both discovery -problems with no heuristics — including the draft's "most recent successful run wins" -tie-break, which is gone: re-dispatching the same version force-pushes a fresh branch, and the -tag pins exactly one `B`. - -Residual trust, stated honestly: someone with push access could hand-craft a `B` (including, -since tag-push workflows execute the YAML at the tagged commit, a `B` that tampers with -`release-publish.yml` itself — which check 4 forbids, but a tampered workflow would not enforce -check 4). Checks 1–6 make such a commit fail loudly against any honest workflow copy; what -closes the loop is that nothing publishes until an admin reviews the release PR (step 3, where a -workflow edit is glaring) and pushes the protected tag. That is the same trust anchor the first -draft placed in "review and merge the PR" — unchanged, since this design kept that step. - -## 6. Artifacts and retention - -`release-build` uploads, per target, a single named artifact **`mx-release--`** -(e.g. `mx-release-swift-`; the target name is in the key so a future Conan target cannot -collide with Swift's): the built archive (the XCFramework zip), its `.sha256` file, and a small -manifest fragment recording the SHA and the platforms/arches built. Retention is set explicitly -to **`retention-days: 90`** rather than inherited from the repo default, so the window is a -documented contract. - -The first draft re-uploaded artifacts under a second workflow run to re-key them to `B` on a -fresh 90-day clock; with the run ID pinned in `B`'s manifest there is nothing to re-key, and a -single window remains: **the tag must be pushed within 90 days of the build.** That is more than -sufficient for this project's cadence; expiry recovery is §11. - -## 7. Versioning and tags - -The version is chosen once, at `release-build` dispatch, as a required input. It has to be: the -release-asset URL is a function of the tag name, and it is baked into `B`'s `Package.swift`, -which must exist *before* the tag does. (The first draft called the tag "the sole source of -truth" while also requiring the URL to be written at build time, with no version input anywhere -— a contradiction; no step in its flow ever supplied the version to the thing writing the URL.) - -The tag remains the **approval**: nothing publishes until `v` is pushed at `B`, and -`release-publish` hard-fails unless the tag name matches the manifest's version exactly (§5, -check 1), so the tag and the baked URL cannot desync. Format `vMAJOR.MINOR.PATCH` (e.g. -`v1.4.0`), or `vMAJOR.MINOR.PATCH-prerelease` for a release candidate (e.g. `v0.6.0-rc1`). -There is still no `VERSION` file and no version field in `Package.swift`; `release-publish` -derives the version string it passes to release notes (and a future Conan target) from -`github.ref_name`. - -**Release candidates.** A version may carry a SemVer 2.0 pre-release suffix: a hyphen, then -dot-separated groups of letters, digits and hyphens. `0.6.0-rc1` is the shape this repository -uses. SemVer sorts a pre-release *before* the version it leads to, so `0.6.0-rc1` comes before -`0.6.0-rc2`, which comes before `0.6.0`. `release-build` rejects a numeric group with a leading -zero for that same ordering reason, and rejects build metadata (`+meta`) because SemVer ignores -it when comparing versions — two releases differing only there would look like one version to a -consumer. - -A candidate goes through the same steps as any other release: the same four human steps, the -same two-commit PR, the same merge commit, the same tag at `B`, and the same six checks at -publish time. Only the last step differs. `release-publish` passes `--prerelease` to -`gh release create` when the version carries a suffix, so the repository's "latest release" -keeps pointing at the newest finished version and a candidate is never the default download. - -A consumer takes a candidate by pinning it: - -```swift -.package(url: "https://github.com/webern/mx.git", exact: "0.6.0-rc1") -``` - -A range requirement such as `from: "0.6.0"` is not the way to reach one. Someone testing a -candidate should be saying so in their manifest, and should have to edit that line to move on to -the finished release. - -Release tags are protected by a repository ruleset on pattern `v*`: **Restrict updates** and -**Restrict deletions**, with repo admins on the bypass list. This is complementary to — not a -substitute for — the tag-push-is-approval decision: it prevents *post-hoc tampering* (a -force-push retargeting `v1.4.0` at a different commit after it already shipped), which would -desync the public release from what the tag claims. It does not add a pre-publish review gate. - -Once the PR is merged and the tag is pushed, the `release-prep/v` branch can be -deleted: `B` stays reachable from both `main` and the tag. - -## 8. Build mechanics (Swift target) - -`mx` builds with CMake/Make, not an Xcode project, and the design keeps it that way. Compilation -uses CMake's built-in Apple cross-compilation support, invoked once per slice — -`CMAKE_SYSTEM_NAME=iOS` plus the slice's `CMAKE_OSX_SYSROOT`/`CMAKE_OSX_ARCHITECTURES` (the -design anticipated a third-party toolchain file, e.g. `leetal/ios-cmake`; none turned out to be -needed). CMake drives `clang`/`clang++` **directly** (the same compiler the existing native -`macos` CI job uses) — no `.xcodeproj` is generated and no `xcodebuild` runs for *compilation*. -Each slice builds the `mx` CMake target and its dependencies (`mx_core`, `mxpugi`) and merges -the three static libraries into one with `libtool -static`, so each XCFramework slice carries a -single library plus the public headers (`src/include`). No module map ships: the consumers are -C++/ObjC++ and include `mx/api/...` via the header search path SwiftPM derives from the bundle; -Swift-`import` support would be additive later. Deployment floors are macOS 11.0 and iOS 13.0. - -The single unavoidable Apple-tool call is the final `xcodebuild -create-xcframework`, used purely -to assemble the three static-lib slices plus headers into a valid `.xcframework` bundle — there -is no vendor-neutral tool that writes that bundle format correctly. Shape: - -``` -3× (CMake + clang build → static .a + headers) # macOS universal, iOS device, iOS simulator -1× xcodebuild -create-xcframework # stitch the three slices -1× zip + swift package compute-checksum # produce the release artifact + checksum -``` - -Platform/arch scope for v1: **macOS** (`arm64`+`x86_64`), **iOS device** (`arm64`), **iOS -simulator** (`arm64`+`x86_64`). tvOS, watchOS, and visionOS are explicitly out of scope for v1. - -Two `Package.swift` manifests exist in this design, and the binary one lives at exactly one -commit per release: - -- **`main`'s manifest** stays a source-only package: sibling-checkout development - (`.package(path:)`) and any non-Apple consumption keep working with zero setup, at every - commit of `main` (`R` guarantees this — the binary manifest never survives to a `main` tip). - The env-gated `MX_BINARY_RELEASE` arm that used to `fatalError` was superseded by this design - and was deleted when the workflows landed — `main` never switches to binary mode, so the arm - was dead code under the final scheme. -- **The release manifest**, generated by `_release-target-swift.yml` from a template checked in - at `A` (`.github/release/Package.swift.template`, with the URL and checksum interpolated) and - existing only at `B`: `.binaryTarget(name: "Mx", url: , checksum: - )` is the **default**, so a `from: "1.4.0"` consumer gets the binary with no - environment setup — the fix for the first draft's fatal assumption that consumers could be - asked to set an environment variable. The source target remains reachable from the release - manifest as the automatic arm on non-Apple hosts (where an XCFramework `binaryTarget` cannot - resolve; the manifest is Swift code evaluated on the consumer's machine, so `#if os(...)` - handles this) and behind an explicit `MX_SOURCE_BUILD=1` opt-out for anyone who wants to - compile a tagged release from source on a Mac. The template's source arms carry a copy of the - source target, so the template must be kept in sync with `Package.swift` whenever the target - changes — both files carry a KEEP IN SYNC comment pointing at each other, the release PR - review is the human check, and the build evaluates the interpolated manifest with - `swift package dump-package` before committing it. - -## 9. The release-target contract (Conan honesty) - -Each release target is a reusable workflow (`on: workflow_call`) — reusable workflows, not -composite actions, because only reusable workflows can contain multiple jobs (a build matrix plus -packaging). The shared contract that keeps the system multi-target: - -- **Inputs:** the commit `sha` to build and the `version` being released. -- **Outputs:** (a) a named artifact `mx-release--` containing the publishable - archive(s) + checksums + a manifest fragment; (b) the exact release-metadata file contents to - commit in `B`; (c) the list of paths those files may occupy — the target's contribution to the - allowed-path set that `release-publish` enforces on the `A..B` diff. - -`release-build` fans out across targets and its prep job merges every target's files into the -one commit `B` (and mechanically reverts the tree-affecting ones in `R`); `release-publish` -validates the diff against the union of all targets' declared paths (clamped by its static -ceiling, §5) and publishes every target's artifact under the one tag. Adding Conan is: write -`_release-target-conan.yml` to that contract, register it in the fan-out, and add its metadata -paths to the publish ceiling. No other change to the publish or validation logic. Conan's actual -recipe/upload mechanics are deliberately left unspecified here. - -## 10. Token permissions (least privilege, per job) - -| Job | Permissions | -|---|---| -| `_release-target-*` build jobs | `contents: read` | -| `release-build` prep job | `contents: write` (pushes the `release-prep/v*` branch), `pull-requests: write` (opens the release PR) | -| `release-publish` | `contents: write` (the only job that can create a Release), `actions: read` (reads the pinned run's metadata and downloads its artifacts — cross-run artifact access requires it; the first draft's table omitted `actions: read` entirely, so both of its artifact-reading jobs would have been denied by the API) | - -No workflow requests broad repo-wide write. The single job that can mint a public Release is -isolated behind the tag-push trigger. - -## 11. Failure modes - -- **Any `release-publish` verification failure** (§5: missing or malformed manifest — including - a tag pushed at `A` or any non-release commit; tag/version mismatch; parentage violation — - including tagging `R` or the merge commit by mistake; `B` not in `main`'s ancestry — the PR - unmerged, or squash/rebase-merged; diff violation; pinned run missing, failed, or foreign; - artifact expired past 90 days; checksum mismatch): **hard-fail, loudly, no fallback and no - auto-rebuild.** Publishing is all-or-nothing; a partial publish is never emitted. The failure - message names the check that tripped and directs the owner to recover by deleting the tag, - fixing the cause (merging the PR when it wasn't; the `-s ours` history-only merge when it was - squashed, §3; re-dispatching `release-build` when the artifact expired or the run was bad — - producing a fresh PR and a fresh `B` to tag), and re-pushing. -- **Commits landing on `main` between `A` and the tag:** harmless. `B` sits directly on `A`, so - mainline activity cannot leak into the tagged tree; at worst the release PR needs a trivial - conflict resolution in `release-manifest.json`, which nothing else touches. (In the first - draft this was a silent freeze requirement that would have hard-failed any release with a - concurrent merge.) -- **No manual override / `force_rebuild` escape hatch exists**, by design — it would silently - reintroduce the rebuild-drift risk the whole two-phase split exists to prevent. - -## 12. Out of scope / future work - -- Conan target implementation (§9 defines only its contract). -- tvOS/watchOS/visionOS slices. -- Code signing / notarization — not applicable to a static-library XCFramework; would only matter - if `mx` later ships a signed executable. -- Auto-generated release notes are GitHub's default categorization of merged PRs since the prior - tag; a curated `CHANGELOG.md`-driven flow is not adopted for v1. - -## 13. Revision log - -**2026-08-23 (d) — release candidates.** A version may now carry a SemVer pre-release suffix, -so `0.6.0-rc1` can be dispatched, tagged and published (§7). Two changes carry it: -`release-build` validates the version against SemVer 2.0 minus build metadata instead of three -plain numbers, and `release-publish` marks the GitHub Release as a pre-release when the version -carries a suffix, which keeps "latest" on the newest finished version. Nothing else moved. The -`v*` tag trigger, the six checks, the asset URL and the two-commit PR shape were all written in -terms of the version string rather than its shape, so they took a candidate without edits. - -**2026-08-23 (c) — implemented.** The workflows in §4, the template in §8, and the -`Package.swift` cleanup shipped in one commit. Implementation drift from the design text, each -recorded in place above: CMake's built-in `CMAKE_SYSTEM_NAME=iOS` support replaced the -anticipated third-party toolchain file, and each slice's three static libraries are merged with -`libtool -static` (§8); the §9 contract's outputs (b) and (c) are concretely a per-target -`mx-release--metadata-` artifact consumed by the prep job (§4); the release PR runs -no CI because it is pushed with the workflow token (§4); and two repository settings became -explicit prerequisites — Actions may create PRs, and "Create a merge commit" is an allowed merge -method (§4). - -**2026-08-23 (b) — owner requirement: tags live on `main`.** The first revision fixed the -"binary-default vs source-default" conflict and the frozen-`main` problem by tagging a commit on -a never-merged release branch. The owner requires release tags to be part of `main`'s history. -Resolution: the release PR returns, now carrying two machine-written commits — `B` (binary -manifest + provenance record; the tag target) and `R` (restores the source manifest) — so a -merge-commit merge places `B` in `main`'s ancestry while every `main` tip stays a source -package (§3). Both original fixes survive: the binary default still exists at exactly one -commit, and `B`'s parent is still `A`, so nothing about `main`'s movement affects the tagged -tree. New consequences, both enforced by §5 check 3: the release PR must be merged with a true -merge commit (squash/rebase would strand `B` outside `main`'s history; recovery documented in -§3), and the tag can only be pushed after the merge. `release-build` regains -`pull-requests: write`. - -**2026-08-23 (a) — design review of the first draft found seven flaws; all remain fixed above.** - -1. *The `head_sha` lookup could never work.* A `pull_request: closed`-triggered run reports the - PR branch head as its `head_sha`, never the merge commit, so `release-publish` querying for a - validating run keyed to the tagged SHA would find nothing, every time. → Run-history queries - are gone; `B` pins its builder's run ID in `release-manifest.json` and publish re-verifies - from primary sources (§5). -2. *The version was unknown when the asset URL had to be written.* The tag was "the sole source - of truth," chosen at step 4, yet the URL derived from it was needed at step 2. → `version` is - a required `release-build` input; publish enforces tag == manifest version (§7). -3. *Consumers could never reach binary mode.* The env-gated `MX_BINARY_RELEASE` arm is - unreachable for downstream SwiftPM consumers, who cannot be made to set environment - variables. → The release manifest, existing only at `B`, is binary-by-default; `main` stays - source-only at every tip (§3, §8). -4. *The merge-based flow silently required a frozen `main`.* Any commit landing between `A` and - the release-prep merge made the merged tree diverge from `A` + metadata. → The tagged commit - is `B`, whose parent is always `A`; the merge commit is never tagged, so `main`'s movement - is irrelevant to the tagged tree (§3). -5. *The permissions table omitted `actions: read`,* which cross-run artifact download requires; - both artifact-reading jobs would have 403'd. → Added (§10). -6. *Nothing specified how the `release-build` run ID was discovered* (dispatch inputs are not - queryable via the API). → The run pins its own `github.run_id` into `B`'s manifest (§5). -7. *The written checksum was never verified against the artifact,* so a corrupted or tampered - checksum would ship a release every consumer fails to resolve. → Publish recomputes the - SHA-256 and requires bytes == manifest == `Package.swift` (§5, check 6). - -Also: per-target artifact names now include the target (`mx-release--`) so a second -target cannot collide with Swift's artifact key (§6). diff --git a/docs/ai/design/xml-backend.md b/docs/ai/design/xml-backend.md index ae1f3728f..9496b514a 100644 --- a/docs/ai/design/xml-backend.md +++ b/docs/ai/design/xml-backend.md @@ -7,7 +7,7 @@ Tracking issue: webern/mx#478. `mx::core` is generated code that reads and writes an XML DOM. Until 0.6 that DOM was pugixml's, called directly: every generated parser took a `pugi::xml_node`, every serializer appended to one, and `mx::core::parse` took a `pugi::xml_document`. That was the right call when the typed model was -designed (see `mx-core-plan.md` §2.1: the earlier `ezxml` virtual-interface wrapper allocated a +designed (the earlier `ezxml` virtual-interface wrapper allocated a `shared_ptr` and copied a `std::string` per node access, and retiring it made the core fast). It left two problems for people who build mx into something larger. @@ -15,14 +15,14 @@ left two problems for people who build mx into something larger. `pugi::` symbol, one from their copy and one from mx's vendored copy. Whether the linker complained or silently picked one depended on versions and build flags. 2. **Consumers who want another XML library.** Some projects standardize on libxml2, tinyxml2 or a - platform parser, and do not want a second XML library in the binary at all. + platform parser, and do not want a second XML library in the binary. ## Step one: the rename (PR #479) The vendored pugixml moved to `src/private/mxpugi/` and was compiled under the `mxpugi` namespace (`scripts/vendor_pugixml.py` does the renaming; `make vendor-pugixml VERSION=x.y` refreshes it). That ended the collisions: mx does not define a `pugi::` symbol, and `make symbol-check` fails CI -if one ever appears. It did nothing for problem 2, and it left `mxpugi::` spelled across the generated +if one ever appears. It did not solve problem 2, and it left `mxpugi::` spelled across the generated code, the runtime, `mx::api` and the tests. ## Step two: the seam (this document) @@ -31,7 +31,7 @@ Everything on mx's side of the XML library now goes through three value types in `src/private/mx/xml/Xml.h`: - `mx::xml::Node` and `mx::xml::Attribute` are trivially copyable handles into a tree. Copying one - copies the reference, the way copying a pointer does. Each wraps the backend's own handle type + copies the reference, the way copying a pointer does. Each wraps the backend's handle type (`Backend::NodeHandle`, `Backend::AttributeHandle`) and every member is an inline, non-virtual one-line forwarder to a static function of `mx::xml::Backend`. - `mx::xml::Document` owns a `Backend::DocumentHandle`. It loads from a file, a string or a stream, @@ -61,7 +61,7 @@ a message that names the requirement. The surface is exactly what `mx::core`, `mx::api` and the tests use. Strings come back as `const char *` owned by the tree, as pugixml returns them, so a name or value lookup never -allocates. Strings go in as `std::string_view`. The seam does not expose the backend's own +allocates. Strings go in as `std::string_view`. The seam does not expose the backend's handle, so nothing on the mx side can come to depend on one backend by accident. When a member of the seam loses its last caller, it goes, and so does its line in the concept: a smaller contract is less for a backend author to write. @@ -114,7 +114,9 @@ canonicalizers, `IdSchemaDriftTest` reading the XSD). When a test needs to hand `mx::core` or read what it wrote, it crosses the seam as text: save the mxpugi document to a string, load the string into an `mx::xml::Document`, call the core, serialize into an `mx::xml::Document`, save that to a string, load it back into mxpugi. The corert harness does exactly this -(`CoreRoundtripImpl.cpp`, steps 3 and 4). A test that only needs the core's view of a document uses +(`CoreRoundtripImpl.cpp`, steps 3 and 4). + +A test that only needs the core's view of a document uses `mx::xml::Document` directly. Test code uses one or the other and never casts between them, which is what a consumer who brings a different XML library would have to do. @@ -124,28 +126,45 @@ The choice is the CMake cache variable `MX_XML_BACKEND`, with three values (`set_property(CACHE ... STRINGS)` lists them; anything else is a configure error). CMake prints the chosen value as `mx: MX_XML_BACKEND=...`. -- `vendored` (the default, always; never chosen by detecting another pugixml target): the pugixml - adapter against mx's private `mxpugi` copy. `mx_core` links `mxpugi` and does not get an extra - definition, so this mode is byte-for-byte the build mx had before the option existed. No - `pugi::` symbol; nothing to install. -- `pugixml`: the pugixml adapter against the consumer's own pugixml. `MX_PUGIXML_TARGET` (cache - string, default `pugixml`, which is the target upstream pugixml's CMake defines as an interface - over `pugixml-static`) names the CMake target; it has to exist when `add_subdirectory(mx)` runs, - and the configure fails with a `FATAL_ERROR` telling the consumer to create it (FetchContent, - `find_package`, or `add_subdirectory` on a checkout) otherwise. `mx_core` links that target - `PUBLIC` and gets the `PUBLIC` compile definitions `MX_XML_PUGI_NAMESPACE=pugi` and - `MX_XML_PUGI_HEADER="pugixml.hpp"`. `mxpugi` is not linked into `mx_core` or `mx`. -- `custom`: the consumer provides `MX_XML_BACKEND_HEADER` (the include path of a header that - defines `struct mx::xml::Backend` satisfying `XmlBackend`, written the way it would appear - between the quotes of an `#include`) and `MX_XML_BACKEND_TARGET` (a CMake target, INTERFACE or - library, carrying the header's include directory, any sources, and the link to the XML library; - it exists before `add_subdirectory(mx)`, so a source on it that includes `mx/xml/Concept.h` has - to carry mx's `src/private` include path itself). - Both are required; the configure fails naming them otherwise. `mx_core` links the target - `PUBLIC`, gets the `PUBLIC` compile definition `MX_XML_BACKEND_HEADER="..."`, and does not - compile `src/private/mx/xml/pugixml/Backend.cpp` (it includes pugixml). The `static_assert` in - `Xml.h` is the check that the struct satisfies the concept. This mode is a documented contract: - there is no CI job and no example backend beyond the pugixml one. +### `vendored` + +The default, always; it is never chosen by detecting another pugixml target. The pugixml adapter +runs against mx's private `mxpugi` copy. `mx_core` links `mxpugi` and does not get an extra +definition, so this mode is byte-for-byte the build mx had before the option existed. No `pugi::` +symbol; nothing to install. + +### `pugixml` + +The pugixml adapter runs against the consumer's pugixml, not the vendored mxpugi. +`MX_PUGIXML_TARGET` (cache string, default `pugixml`, which is the target upstream pugixml's CMake +defines as an interface over `pugixml-static`) names the CMake target. + +The target has to exist when `add_subdirectory(mx)` runs. Otherwise the configure fails with a +`FATAL_ERROR` telling the consumer to create it (FetchContent, `find_package`, or +`add_subdirectory` on a checkout). + +`mx_core` links that target `PUBLIC` and gets the `PUBLIC` compile definitions +`MX_XML_PUGI_NAMESPACE=pugi` and `MX_XML_PUGI_HEADER="pugixml.hpp"`. `mxpugi` is not linked into +`mx_core` or `mx`. + +### `custom` + +The consumer provides two settings, and the configure fails naming them if either is missing: + +- `MX_XML_BACKEND_HEADER`: the include path of a header that defines `struct mx::xml::Backend` + satisfying `XmlBackend`, written the way it would appear between the quotes of an `#include`. +- `MX_XML_BACKEND_TARGET`: a CMake target, INTERFACE or library, carrying the header's include + directory, any sources, and the link to the XML library. It exists before + `add_subdirectory(mx)`, so a source on it that includes `mx/xml/Concept.h` has to carry mx's + `src/private` include path itself. + +`mx_core` links the target `PUBLIC`, gets the `PUBLIC` compile definition +`MX_XML_BACKEND_HEADER="..."`, and does not compile `src/private/mx/xml/pugixml/Backend.cpp` (it +includes pugixml). The `static_assert` in `Xml.h` is the check that the struct satisfies the +concept. This mode is a documented contract: there is no CI job and no example backend beyond the +pugixml one. + +### All modes The link and the definitions are `PUBLIC` on `mx_core` because `Xml.h` is included by the core's headers: the `mx` library, the tests and the consumer all have to see the same backend. @@ -162,10 +181,13 @@ default `MX_API=ON` build of everything still does, because `mxtest` links it). The `pugixml` mode has a gate. `cmake/backend-pugixml/CMakeLists.txt` is a consumer project that fetches pugixml 1.16 with FetchContent, sets `MX_XML_BACKEND=pugixml`, `MX_CORE_DEV=ON` and `MX_API=OFF` (the backend lives in `mx_core`; the core suites are what runs), and -adds the repository root as a subdirectory. `make backend-pugixml-test` configures it into +adds the repository root as a subdirectory. + +`make backend-pugixml-test` configures it into `build/backend-pugixml`, builds it, runs the corert and core unit binaries, and then checks with `nm -g | c++filt` that `libmx_core.a` in that tree references `pugi::` and does not hold an `mxpugi::` symbol, which is the proof that the mode took. The CI job `backend: pugixml` runs that target. + `make symbol-check` is the gate for the vendored build only; in the pugixml mode `mx_core` references `pugi::` on purpose. diff --git a/gen/cpp/config.toml b/gen/cpp/config.toml index 2923a5b06..62060c840 100644 --- a/gen/cpp/config.toml +++ b/gen/cpp/config.toml @@ -45,7 +45,7 @@ doctype-timewise = 'score-timewise PUBLIC \"-//Recordare//DTD MusicXML 4.0 Timew [naming] extends = "../naming.base.toml" # schema-forced renames shared by all targets -# Modern accessor style (plan §2.6): camelCase accessors `step()` / +# Modern accessor style: camelCase accessors `step()` / # `setStep(...)`; the templates spell private members `m_{{ident}}`. field-convention = "camel" # Variant identifiers are the named-factory spellings on the enum wrapper @@ -64,7 +64,7 @@ string = "std::string" token = "std::string" date = "std::string" # The NCName-derived identity primitives get hand-written repairing wrappers -# (valid-by-construction, plan §2.2): a lexically invalid id/idref/nmtoken +# (valid-by-construction): a lexically invalid id/idref/nmtoken # cannot exist through the public API. Token enforces NCName (xs:ID/IDREF); # NameToken enforces the wider xs:NMTOKEN (leading digits allowed). nmtoken = "NameToken" @@ -103,7 +103,7 @@ words = [ "small", ] -# Semantic names for the synthesized content types the §2.9 projection +# Semantic names for the synthesized content types the grammar-preserving projection # hoists out of anonymous nested particles (mechanical wires like # `note-choice-group-2` stay addressable; these give the high-traffic note # and lyric structures their domain names). @@ -119,7 +119,7 @@ words = [ "lyric-choice-group-group-group" = "elision-syllabic-group" [import] -# Import-policy repairs (plan §2.4): a required attribute missing from a +# Import-policy repairs: a required attribute missing from a # document gets the configured default injected by the parser; missing with # no entry is a parse error. xml:lang on lyric-language defaults to BCP 47 # "und" (undetermined). @@ -129,7 +129,7 @@ words = [ # ---- the render manifest: which templates produce which files ----------- # # One template per shape (value shapes here; the complex/group/choice shapes # land with the W2 templates). Bespoke rows give individual value types the -# structural representations of mx-core-plan.md §2.2. +# structural representations they need. [render] dir = "templates" @@ -178,7 +178,7 @@ strategies = ["tagged-variant"] template = "union.cpp.tmpl" output = "{pascal}.cpp" -# ---- bespoke structural value types (plan §2.2) -------------------------- # +# ---- bespoke structural value types -------------------------------------- # [[render.type]] types = ["color"] @@ -349,8 +349,8 @@ strategies = ["choice-class"] template = "choice.cpp.tmpl" output = "{pascal}.cpp" -# Measure music-data is the known large-alternative repeated choice (plan -# §2.3): its alternatives are boxed so a by-value variant does not size +# Measure music-data is the known large-alternative repeated choice: its +# alternatives are boxed so a by-value variant does not size # every slot at the largest alternative. Which types box is this manifest # data, not template hardcode. [[render.type]] diff --git a/gen/cpp/templates/attributes_parse.tmpl b/gen/cpp/templates/attributes_parse.tmpl index cbf59227c..751450272 100644 --- a/gen/cpp/templates/attributes_parse.tmpl +++ b/gen/cpp/templates/attributes_parse.tmpl @@ -32,7 +32,7 @@ if (!seen_{{ident}}) { {{#import_default}} - // Configured import repair (plan §2.4): inject the default. + // Configured import repair: inject the default. out.set{{name.pascal}}({{>attr_default_expr}}); reportAttributeDefaulted(context, el, {{name.wire_q}}, {{import_default_q}}); {{/import_default}} diff --git a/gen/cpp/templates/choice.h.tmpl b/gen/cpp/templates/choice.h.tmpl index a80e47672..bf5bdd941 100644 --- a/gen/cpp/templates/choice.h.tmpl +++ b/gen/cpp/templates/choice.h.tmpl @@ -33,7 +33,7 @@ class ParseContext; /// A schema choice{{#synthesized}} (synthesized from an anonymous particle /// of the schema){{/synthesized}}: exactly one alternative by construction. /// Alternatives are positional; dispatch is by Kind/index, never by type -/// (alternative types may repeat -- plan §2.3). +/// (alternative types may repeat). class {{ident}} final { public: diff --git a/gen/cpp/templates/choice_boxed.h.tmpl b/gen/cpp/templates/choice_boxed.h.tmpl index c2f346ba9..52b912416 100644 --- a/gen/cpp/templates/choice_boxed.h.tmpl +++ b/gen/cpp/templates/choice_boxed.h.tmpl @@ -25,7 +25,7 @@ class ParseContext; {{/doc_lines}} /// A repeated heterogeneous choice with large alternatives: each is boxed /// so a by-value variant does not size every slot at the largest -/// alternative (plan §2.3). The API stays value-like; copies clone. +/// alternative. The API stays value-like; copies clone. class {{ident}} final { public: diff --git a/gen/cpp/templates/color.h.tmpl b/gen/cpp/templates/color.h.tmpl index 77dd691bd..6aaf9649f 100644 --- a/gen/cpp/templates/color.h.tmpl +++ b/gen/cpp/templates/color.h.tmpl @@ -15,7 +15,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2): numeric RGB components plus an +/// Structural representation: numeric RGB components plus an /// optional alpha; serialization formats #RRGGBB / #AARRGGBB. A malformed /// hex string cannot exist. class {{ident}} final diff --git a/gen/cpp/templates/comma_separated_text.h.tmpl b/gen/cpp/templates/comma_separated_text.h.tmpl index ff6350e5a..6743b833d 100644 --- a/gen/cpp/templates/comma_separated_text.h.tmpl +++ b/gen/cpp/templates/comma_separated_text.h.tmpl @@ -15,7 +15,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2): a never-empty list of non-empty, +/// Structural representation: a never-empty list of non-empty, /// comma-free items. Construction repairs deterministically (commas inside /// an item become spaces, blank items drop, an empty list becomes ["-"]). /// A value parsed from the wire keeps its exact spelling (the separator may diff --git a/gen/cpp/templates/composite.h.tmpl b/gen/cpp/templates/composite.h.tmpl index cd5b6cff1..4c02b3816 100644 --- a/gen/cpp/templates/composite.h.tmpl +++ b/gen/cpp/templates/composite.h.tmpl @@ -41,7 +41,7 @@ class ParseContext; /// {{value}} {{/doc_lines}} /// Content fields mirror the schema grammar in declaration order; the -/// serializer walks them, so a wrong order is unrepresentable (plan §2.3). +/// serializer walks them, so a wrong order is unrepresentable. class {{ident}}{{^is_base}} final{{/is_base}} { public: diff --git a/gen/cpp/templates/derived.h.tmpl b/gen/cpp/templates/derived.h.tmpl index 5c13ecfd7..ffc227c1e 100644 --- a/gen/cpp/templates/derived.h.tmpl +++ b/gen/cpp/templates/derived.h.tmpl @@ -33,7 +33,7 @@ class ParseContext; /// {{value}} {{/doc_lines}} /// The schema's complexContent extension: non-polymorphic public -/// inheritance, no virtual anything (plan §2.3). +/// inheritance, no virtual anything. class {{ident}} : public {{base.ident}} { public: diff --git a/gen/cpp/templates/document.h.tmpl b/gen/cpp/templates/document.h.tmpl index 1c717473b..f25808705 100644 --- a/gen/cpp/templates/document.h.tmpl +++ b/gen/cpp/templates/document.h.tmpl @@ -89,13 +89,13 @@ private: std::vector m_rootNamespaces; }; -/// The parse error boundary (plan §2.5): strict on names and structure, +/// The parse error boundary: strict on names and structure, /// lenient on values; a root declaring a version newer than /// SupportedMusicXMLVersion is rejected with unsupportedVersion. Result parse(const mx::xml::Document &doc, const ParseContext &context); // TODO: document the unique ID exception -/// Total for every Document (§1.1 guarantees validity): writes the XML +/// Total for every Document (a Document is valid by construction): writes the XML /// declaration, the matching DOCTYPE, the preserved root namespace /// declarations, and the typed tree. void serialize(const Document &d, mx::xml::Document &out); diff --git a/gen/cpp/templates/empty.h.tmpl b/gen/cpp/templates/empty.h.tmpl index 89695c55b..86e3f834b 100644 --- a/gen/cpp/templates/empty.h.tmpl +++ b/gen/cpp/templates/empty.h.tmpl @@ -17,7 +17,7 @@ class ParseContext; {{/doc_lines}} /// Presence-only marker: the element's only information is whether it /// appears. Optional occurrences project to bool fields; choice -/// alternatives hold this marker (plan §2.3). +/// alternatives hold this marker. class {{ident}}{{^is_base}} final{{/is_base}} { public: diff --git a/gen/cpp/templates/ending_number.h.tmpl b/gen/cpp/templates/ending_number.h.tmpl index bad22539f..4c941aad7 100644 --- a/gen/cpp/templates/ending_number.h.tmpl +++ b/gen/cpp/templates/ending_number.h.tmpl @@ -15,7 +15,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2): the ending numbers as a list of +/// Structural representation: the ending numbers as a list of /// positive integers, possibly empty (the schema's blank-ending case, used /// when the number is not printed). Construction clamps each value to >= 1. /// A value parsed from the wire keeps its exact spelling (blank endings may diff --git a/gen/cpp/templates/fields_api.tmpl b/gen/cpp/templates/fields_api.tmpl index a999b17ee..f73c83e50 100644 --- a/gen/cpp/templates/fields_api.tmpl +++ b/gen/cpp/templates/fields_api.tmpl @@ -17,7 +17,7 @@ std::span {{ident}}() const noexcept; {{#max}} /// Bounded by the schema: at most {{max}} occurrences. An over-cap - /// append is rejected (plan §1.2). + /// append is rejected. [[nodiscard]] Result add{{name.pascal}}({{type_ref.ident}} value); void clear{{name.pascal}}() noexcept; {{/max}} diff --git a/gen/cpp/templates/group.h.tmpl b/gen/cpp/templates/group.h.tmpl index 061f752e3..9ed4a3e2f 100644 --- a/gen/cpp/templates/group.h.tmpl +++ b/gen/cpp/templates/group.h.tmpl @@ -35,7 +35,7 @@ class ParseContext; {{/doc_lines}} /// A shared content group{{#synthesized}} (synthesized from an anonymous /// particle of the schema){{/synthesized}}: transparent on the wire, its -/// fields serialize directly into the referencing element (plan §2.3). +/// fields serialize directly into the referencing element. class {{ident}} final { public: diff --git a/gen/cpp/templates/number.h.tmpl b/gen/cpp/templates/number.h.tmpl index 6d821926f..f9fe30829 100644 --- a/gen/cpp/templates/number.h.tmpl +++ b/gen/cpp/templates/number.h.tmpl @@ -44,7 +44,7 @@ public: std::string toString() const; /// Strict lexical parse, then range-clamp (the import leniency policy - /// for numbers, plan §2.4). + /// for numbers). static bool tryParse(std::string_view text, {{ident}} &out); /// Lenient: non-numeric text yields the clamped zero. diff --git a/gen/cpp/templates/smufl_prefixed.h.tmpl b/gen/cpp/templates/smufl_prefixed.h.tmpl index cf841508f..8908c72d8 100644 --- a/gen/cpp/templates/smufl_prefixed.h.tmpl +++ b/gen/cpp/templates/smufl_prefixed.h.tmpl @@ -13,7 +13,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2): the glyph-name prefix is +/// Structural representation: the glyph-name prefix is /// structural and emitted by the serializer; only the suffix is stored. /// Construction repairs the suffix to name characters{{#suffix_required}} /// (and to non-empty, the placeholder being "X"){{/suffix_required}}, so a diff --git a/gen/cpp/templates/smufl_wavy.h.tmpl b/gen/cpp/templates/smufl_wavy.h.tmpl index 9e8b8be68..6a1bce444 100644 --- a/gen/cpp/templates/smufl_wavy.h.tmpl +++ b/gen/cpp/templates/smufl_wavy.h.tmpl @@ -13,7 +13,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2) of the two branches the pattern +/// Structural representation of the two branches the pattern /// allows: wiggle (suffix non-empty), or guitarVibratoStroke /// (middle possibly empty). The literals are structural; only the variable /// part is stored, so a pattern-invalid name cannot exist. diff --git a/gen/cpp/templates/string.cpp.tmpl b/gen/cpp/templates/string.cpp.tmpl index d015907ea..2a94d4d19 100644 --- a/gen/cpp/templates/string.cpp.tmpl +++ b/gen/cpp/templates/string.cpp.tmpl @@ -10,7 +10,7 @@ namespace {{vars.namespace}} std::string repaired{{ident}}(std::string v) { {{#min_length}} - // Deterministic repair to the schema's minLength (plan §2.2.2): pad + // Deterministic repair to the schema's minLength: pad // with '-' so a too-short value is unrepresentable. while (v.size() < {{min_length}}) { diff --git a/gen/cpp/templates/time_only.h.tmpl b/gen/cpp/templates/time_only.h.tmpl index 24d096bbb..4a11914e8 100644 --- a/gen/cpp/templates/time_only.h.tmpl +++ b/gen/cpp/templates/time_only.h.tmpl @@ -15,7 +15,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2): a never-empty list of positive +/// Structural representation: a never-empty list of positive /// integers (the time-through-repeat indices), serialized comma-separated. /// Construction clamps each value to >= 1; an empty list becomes [1]. class {{ident}} final diff --git a/gen/cpp/templates/yyyy_mm_dd.h.tmpl b/gen/cpp/templates/yyyy_mm_dd.h.tmpl index a8ee7164e..4671fa9a9 100644 --- a/gen/cpp/templates/yyyy_mm_dd.h.tmpl +++ b/gen/cpp/templates/yyyy_mm_dd.h.tmpl @@ -13,7 +13,7 @@ namespace {{vars.namespace}} {{#doc_lines}} /// {{value}} {{/doc_lines}} -/// Structural representation (plan §2.2): year/month/day integers clamped +/// Structural representation: year/month/day integers clamped /// to valid calendar ranges (leap years honored), serialized YYYY-MM-DD. An /// impossible date cannot exist. class {{ident}} final diff --git a/gen/plates/build.py b/gen/plates/build.py index cc7a77b94..8d1aa0e90 100644 --- a/gen/plates/build.py +++ b/gen/plates/build.py @@ -304,7 +304,7 @@ def __init__(self, m: ir.Ir, config: Config): self.type_map = dict(config.types) self.variant_scope = config.target.variant_scope - # The grammar-preserving content projection (plan §2.9): normalize + # The grammar-preserving content projection: normalize # each content tree once (the discovery and the build must see the # SAME particle objects), walk it to find every structural group the # schema actually references and every anonymous choice/sequence @@ -344,7 +344,7 @@ def __init__(self, m: ir.Ir, config: Config): config.target.symbol_prefix + name.cased[naming.type_convention] ) - # ----- content discovery (plan §2.9) -------------------------------------- # + # ----- content discovery -------------------------------------------------- # # # One deterministic preorder walk over every composite's normalized # content, recursing through structural group references (each group @@ -713,7 +713,7 @@ def _complex_plate(self, ct: ir.ComplexType) -> ComplexPlate: plate.structural_deps = _dep_refs(srefs) return plate - # ----- group, choice, and synthesized content plates (plan §2.9) --------- # + # ----- group, choice, and synthesized content plates --------------------- # def _group_plates(self) -> list: out: list = [] @@ -1027,7 +1027,7 @@ def _validate_config_against_ir(self) -> list[str]: errors += self._promotion_errors() # Structural-group and synthesized content-type wires are addressable - # for renames too (plan §2.9). + # for renames too. type_wires = set(self.values_by_name) | set(self.complex_by_name) type_wires |= {self.group_wire[g] for g in self.struct_groups} type_wires |= set(self.synth_content) diff --git a/gen/plates/check.py b/gen/plates/check.py index 9c1304a6e..b6b7b8194 100644 --- a/gen/plates/check.py +++ b/gen/plates/check.py @@ -182,7 +182,7 @@ def _check_members(plates: Plates, errors: list[str]) -> None: def _check_fields(plates: Plates, errors: list[str]) -> None: - """The grammar-preserving projection's identifier scopes (plan §2.9): a + """The grammar-preserving projection's identifier scopes: a structural target's class holds the attribute members AND the content fields, so the two lists must be mutually unique; a choice type's alternative accessors share one scope likewise.""" diff --git a/gen/plates/model.py b/gen/plates/model.py index 9198d82e0..d7fb1a9ec 100644 --- a/gen/plates/model.py +++ b/gen/plates/model.py @@ -260,7 +260,7 @@ class Member: fixed: str | None = None default_variant: str | None = None # The configured import repair for a required attribute MISSING from a - # document ([import] attribute-defaults, plan §2.4): the parser injects + # document ([import] attribute-defaults): the parser injects # this wire literal; without one, missing is a parse error. import_default: str | None = None doc: str | None = None @@ -293,7 +293,7 @@ class ComplexPlate: strategy: str # value-class | composite-class | flag | attrs-class | inherit | flatten members: list[Member] = field(default_factory=list) content: ir.Particle | None = None - # The grammar-preserving field projection of `content` (plan §2.9): + # The grammar-preserving field projection of `content`: # ordered fields a structural target renders instead of the flat # `members` element view. Empty for content-free shapes. fields: list[ContentField] = field(default_factory=list) @@ -334,7 +334,7 @@ def members_view(self) -> list[Member]: # --------------------------------------------------------------------------- # -# Content fields (the grammar-preserving projection, plan §2.9) +# Content fields (the grammar-preserving projection) # --------------------------------------------------------------------------- # @@ -378,7 +378,7 @@ class Alternative: is nullable (the choice can be satisfied empty, e.g. key's non-traditional-key*). Alternatives are positional: a target whose sum type allows duplicate member types must dispatch by index, never by - type (plan §2.3).""" + type.""" name: Name ident: str @@ -472,8 +472,8 @@ class Plates: schema_version: str = "" # the MusicXML version in the source stem ("3.1") value_types: list[ValuePlate] = field(default_factory=list) complex_types: list[ComplexPlate] = field(default_factory=list) - # The shared content types of the grammar-preserving projection (plan - # §2.9): structural named schema groups plus the synthesized types + # The shared content types of the grammar-preserving projection: + # structural named schema groups plus the synthesized types # hoisted out of anonymous nested choices/sequences. A target consuming # only the flat member view never renders these. groups: list[GroupLikePlate] = field(default_factory=list) diff --git a/gen/press/render.py b/gen/press/render.py index bd7440a51..797491bfd 100644 --- a/gen/press/render.py +++ b/gen/press/render.py @@ -33,7 +33,7 @@ } ) -# The grammar-preserving projection's shared content types (plan §2.9). +# The grammar-preserving projection's shared content types. # Rendered only when the manifest engages them, so a target consuming the # flat member view (and its committed output) is untouched by their # existence. diff --git a/scripts/doctor.sh b/scripts/doctor.sh index 34e5b8544..9928583b4 100755 --- a/scripts/doctor.sh +++ b/scripts/doctor.sh @@ -106,20 +106,19 @@ if [ -x "$QUALITY_VENV/bin/python" ]; then else row 'gen-quality, gen-lint' 'MISSING venv; run make quality-venv' fi -row 'fmt, fmt-check' "$(need "$CLANG_FORMAT")" +row fmt "$(need "$CLANG_FORMAT")" row 'build-go, test-go' "$(need go)" c_status=$(need "$CMAKE" "$CC_TOOL") if ! libxml2_found; then if [ "$c_status" = 'ok' ]; then c_status='MISSING libxml2'; else c_status="$c_status libxml2"; fi fi row 'build-c, test-c' "$c_status" -if [ -x "$DESLAG" ]; then - row 'deslag (not used by targets yet)' 'ok' -elif has cargo; then - row 'deslag (not used by targets yet)' 'MISSING; run make deslag' -else - row 'deslag (not used by targets yet)' 'MISSING; make deslag needs cargo' +fmt_check_status=$(need "$CLANG_FORMAT") +if [ ! -x "$DESLAG" ]; then + if has cargo; then deslag_status='MISSING deslag; run make deslag'; else deslag_status='MISSING deslag; make deslag needs cargo'; fi + if [ "$fmt_check_status" = 'ok' ]; then fmt_check_status=$deslag_status; else fmt_check_status="$fmt_check_status, $deslag_status"; fi fi +row fmt-check "$fmt_check_status" if has ccache; then row 'ccache (optional)' 'found; native builds use it' else diff --git a/src/include/mx/api/ApiCommon.h b/src/include/mx/api/ApiCommon.h index e823e1005..385096509 100644 --- a/src/include/mx/api/ApiCommon.h +++ b/src/include/mx/api/ApiCommon.h @@ -71,11 +71,12 @@ enum class MeasureNumbering system }; -// Whether an item is associated with the system rather than only with the part it appears in -- -// the system attribute of (MeasureData) and of and +// Whether an item is associated with the system rather than only with the part it appears in -- the +// system attribute of (MeasureData) and of and // (DirectionData). only... means the item is drawn on the top or bottom part of the system instead // of this part; also... means it is drawn on both. unspecified means the attribute is absent. -// onlyBottom and alsoBottom are measure-numbering only; MusicXML has no bottom-of-system direction. +// onlyBottom and alsoBottom are measure-numbering only; MusicXML does not have a bottom-of-system +// direction. enum class SystemRelation { unspecified, @@ -88,8 +89,9 @@ enum class SystemRelation // The shape drawn around a piece of text or a symbol -- MusicXML's enclosure attribute, carried by // RehearsalData, WordsData, SymbolData, PercussionData and PageTextData. unspecified means the -// attribute is absent, which draws no enclosure; none states explicitly that there is none. A -// bracket is a rectangle with the bottom line missing, as is common in jazz notation, and an +// attribute is absent, which implicitly means there is no enclosure; none states explicitly that +// there is no enclosure. +// A bracket is a rectangle with the bottom line missing, as is common in jazz notation, and an // invertedBracket is one with the top line missing. enum class Enclosure { diff --git a/src/include/mx/api/BarlineData.h b/src/include/mx/api/BarlineData.h index 7dbbc4cc8..3d0e4eb3a 100644 --- a/src/include/mx/api/BarlineData.h +++ b/src/include/mx/api/BarlineData.h @@ -62,17 +62,17 @@ class EndingData std::string text; // Where the bracket is drawn, in tenths of a staff space. defaultY sets the height of the - // bracket line above the top line of the staff, and defaultX moves the start of the bracket left - // or right of the barline. relativeX and relativeY shift the bracket from wherever it would - // otherwise be drawn. Set the matching is...Specified flag along with each value. An ending has - // no placement or alignment, so those members are not written. Leave this alone to let the - // program place the bracket. + // bracket line above the top line of the staff, and defaultX moves the start of the bracket + // left or right of the barline. relativeX and relativeY shift the bracket from wherever it + // would otherwise be drawn. Set the matching is...Specified flag along with each value. An + // ending does not have a placement or alignment, so those members are not written. Leave this + // alone to let the program place the bracket. PositionData positionData; // The length, in tenths of a staff space, of the downward jog at the end of the bracket. A stop - // ending closes with this jog; a discontinue ending has no jog. MusicXML does not say what the - // value means on a start ending, though some programs write one there. Leave it empty for the - // usual length. + // ending closes with this jog; a discontinue ending does not have one. MusicXML does not say + // what the value means on a start ending, though some programs write one there. Leave it empty + // for the usual length. std::optional endLength; // Where the label starts, in tenths of a staff space from the start of the bracket line: textX diff --git a/src/include/mx/api/ChordData.h b/src/include/mx/api/ChordData.h index 812ecc6a6..72f7276e5 100644 --- a/src/include/mx/api/ChordData.h +++ b/src/include/mx/api/ChordData.h @@ -248,7 +248,7 @@ class ChordData int rootAlter; // How the root is spelled in place of its letter name, such as "Do" for C in solfege or "H" for // B in German. Leave absent to print the letter. An empty string is different from absent: it - // prints nothing for the root, and the rest of the chord symbol still prints. Spell only the + // prints an empty root, and the rest of the chord symbol still prints. Spell only the // step, because rootAlter is still printed beside the text: B-flat in solfege is "Si" with a // rootAlter of -1, not "Sib". Used only when harmonyChordSource == root. std::optional rootText; @@ -289,10 +289,10 @@ class ChordData Bool parenthesesDegrees; Step bass; int bassAlter; - // How the bass note after the slash is spelled in place of its letter name, such as "Sol" for the - // G of C/G. Leave absent to print the letter. An empty string is different from absent: it - // prints nothing for the bass note. Spell only the step, because bassAlter is still printed - // beside the text. Used only when bass is set. + // How the bass note after the slash is spelled in place of its letter name, such as "Sol" for + // the G of C/G. Leave absent to print the letter. An empty string is different from absent: it + // prints an empty bass note. Spell only the step, because bassAlter is still printed beside the + // text. Used only when bass is set. std::optional bassText; // The MusicXML (0 = root position, 1 = first inversion, ...). Present only when // hasInversion is true. diff --git a/src/include/mx/api/CodaData.h b/src/include/mx/api/CodaData.h index 1a18b7177..096d934b6 100644 --- a/src/include/mx/api/CodaData.h +++ b/src/include/mx/api/CodaData.h @@ -19,8 +19,8 @@ namespace api { // The MusicXML element carries the empty-print-object-style-align attribute group plus the // smufl glyph-name and optional-unique-id. positionData captures default/relative x-y plus the -// horizontal and vertical alignment; its placement member is unused here because has no -// placement attribute (placement lives on the parent ). +// horizontal and vertical alignment; its placement member is unused here because does not +// have a placement attribute (placement lives on the parent ). class CodaData { public: diff --git a/src/include/mx/api/ComplexTimeSignature.h b/src/include/mx/api/ComplexTimeSignature.h index 8fb681339..70a2b536e 100644 --- a/src/include/mx/api/ComplexTimeSignature.h +++ b/src/include/mx/api/ComplexTimeSignature.h @@ -17,7 +17,7 @@ namespace mx namespace api { -// The full MusicXML time-symbol vocabulary. Used for a complex meter's own symbol and for an +// The full MusicXML time-symbol vocabulary. Used for a complex meter's symbol and for an // interchangeable pair's symbol. Unlike the narrow TimeSignatureSymbol (common/cut only), this // includes the unusual display modes that force a signature into the complex arena. enum class ComplexTimeSymbol @@ -30,8 +30,8 @@ enum class ComplexTimeSymbol dottedNote // the beat-type drawn as a dotted downstem note glyph }; -// The line drawn between beats and beat-type (MusicXML time-separator). 'unspecified' writes no -// attribute (the spec default is "none"). +// The line drawn between beats and beat-type (MusicXML time-separator). 'unspecified' does not +// write the attribute (the spec default is "none"). enum class TimeSeparator { unspecified, @@ -129,7 +129,7 @@ class ComplexTimeSignature // Construct a senza-misura time signature. // - // glyph is free text; MusicXML defines no valid/invalid values beyond xs:string (e.g. "X"). + // glyph is free text; MusicXML does not limit its values beyond xs:string (e.g. "X"). ComplexTimeSignature(std::string glyph); Kind kind() const; diff --git a/src/include/mx/api/DampData.h b/src/include/mx/api/DampData.h index 7870ef57d..f0ef41cfe 100644 --- a/src/include/mx/api/DampData.h +++ b/src/include/mx/api/DampData.h @@ -19,7 +19,7 @@ namespace api { // A harp damping mark, MusicXML's element: the player silences one or more ringing // strings with the hand. positionData captures default/relative x-y plus the horizontal and -// vertical alignment; its placement member is unused here because has no placement +// vertical alignment; its placement member is unused here because does not have a placement // attribute (placement lives on the parent ). class DampData { diff --git a/src/include/mx/api/Diagnostics.h b/src/include/mx/api/Diagnostics.h index 4b351a56e..057b82b45 100644 --- a/src/include/mx/api/Diagnostics.h +++ b/src/include/mx/api/Diagnostics.h @@ -27,7 +27,7 @@ enum class Severity enum class DiagnosticCode { valueAdjusted, // a value was changed to one that MusicXML can represent - unmatchedSpanner, // a spanner endpoint had no matching endpoint + unmatchedSpanner, // a spanner endpoint did not have a matching endpoint invalidValue, // a value could not be read, so a default was used missingValueDefaulted, // a required value was missing, so a default was used droppedData, // data could not be read or written, so it was left out diff --git a/src/include/mx/api/DirectionData.h b/src/include/mx/api/DirectionData.h index 809dee75b..69c88f423 100644 --- a/src/include/mx/api/DirectionData.h +++ b/src/include/mx/api/DirectionData.h @@ -35,7 +35,8 @@ struct DirectionData // draws it only on the top part of the system, alsoTop on both this part and the top part, and // none states explicitly that it belongs to this part alone. One value covers the direction // types and the chords held here; both elements are written with it. SystemRelation::onlyBottom - // and ::alsoBottom exist only for measure numbering; either one here writes no attribute. + // and ::alsoBottom exist only for measure numbering; either one here does not write the + // attribute. SystemRelation systemRelation; // Whether this direction lines up with the measure's time signature rather than with the @@ -47,12 +48,15 @@ struct DirectionData // alone. Bool directive; - // The source's , in divisions, or absent when it had none. An nudges only - // where the direction is *drawn*, shifting it away from the note it is anchored to; it does not - // move that anchor. tickTimePosition holds the anchor -- the musical location the direction - // belongs to -- so the drawn position is tickTimePosition + offset. This is a round-trip - // fidelity knob: leave it absent when authoring and set tickTimePosition to where the direction - // belongs; the reader fills it in only to preserve an already present in the source. + // The source's , in divisions, or absent when the source did not have one. An + // nudges only where the direction is *drawn*, shifting it away from the note it is anchored to; + // it does not move that anchor. tickTimePosition holds the anchor -- the musical location the + // direction belongs to -- so the drawn position is tickTimePosition + offset. + // + // This field exists for round-trip fidelity: leave it absent when authoring and set + // tickTimePosition to where the direction belongs. The reader fills it in only to keep an + // already present in the source. + // // MusicXML Documentation: An offset is represented in terms of divisions, and indicates where // the direction will appear relative to the current musical location. This affects the visual // appearance of the direction. If an element within a direction includes a default-x diff --git a/src/include/mx/api/DynamicsData.h b/src/include/mx/api/DynamicsData.h index c61e97cf2..fc51dc0e6 100644 --- a/src/include/mx/api/DynamicsData.h +++ b/src/include/mx/api/DynamicsData.h @@ -50,8 +50,8 @@ enum class StandardDynamic // The letters of the symbol, e.g. "ff" -- also the name of the MusicXML element that carries it. std::string toString(StandardDynamic value); -// A component of a dynamic mark that has no dedicated MusicXML dynamic element. text is the -// visible fallback; smufl, when present, names the exact glyph to draw. +// A component of a dynamic mark that does not have a dedicated MusicXML dynamic element. text is +// the visible fallback; smufl, when present, names the exact glyph to draw. struct OtherDynamicsData { std::string text; @@ -97,8 +97,8 @@ class DynamicsComponent MXAPI_NOT_EQUALS_AND_VECTORS(DynamicsComponent); -// The component's letters: the symbol for a standard component, the fallback text for one that has -// no dedicated MusicXML element. +// The component's letters: the symbol for a standard component, the fallback text for one that does +// not have a dedicated MusicXML element. std::string toString(const DynamicsComponent &value); // A dynamic mark assembled from multiple symbols in order, such as ff followed by z for ffz. diff --git a/src/include/mx/api/EncodingData.h b/src/include/mx/api/EncodingData.h index cbc44a353..e7ec29ed1 100644 --- a/src/include/mx/api/EncodingData.h +++ b/src/include/mx/api/EncodingData.h @@ -67,7 +67,7 @@ class EncodingData // the bad output back to the exact mx build that produced it. That stamp is // deliberately intrusive -- it injects a node the source never had into every // file mx writes -- so this flag exists to turn it off when byte-faithful output - // is needed (e.g. reproducing a defect without mx's own marker in the diff). + // is needed (e.g. reproducing a defect without mx's marker in the diff). // // Defaults true, including for files parsed without the stamp. When false, // writeToFile/writeToStream omit mx's node; the user's own entries are diff --git a/src/include/mx/api/EyeglassesData.h b/src/include/mx/api/EyeglassesData.h index 28a25140d..f5f71186b 100644 --- a/src/include/mx/api/EyeglassesData.h +++ b/src/include/mx/api/EyeglassesData.h @@ -17,11 +17,11 @@ namespace mx { namespace api { -// The eyeglasses symbol, MusicXML's element: a cue common in commercial music -// telling the player to watch the conductor (or another player) closely at this spot. -// positionData captures default/relative x-y plus the horizontal and vertical alignment; its -// placement member is unused here because has no placement attribute (placement -// lives on the parent ). +// The eyeglasses symbol, MusicXML's element: a cue common in commercial music telling +// the player to watch the conductor (or another player) closely at this spot. positionData captures +// default/relative x-y plus the horizontal and vertical alignment; its placement member is unused +// here because does not have a placement attribute (placement lives on the parent +// ). class EyeglassesData { public: diff --git a/src/include/mx/api/FiguredBassData.h b/src/include/mx/api/FiguredBassData.h index 55f33f23a..79d0b64c4 100644 --- a/src/include/mx/api/FiguredBassData.h +++ b/src/include/mx/api/FiguredBassData.h @@ -50,7 +50,7 @@ class FigureData std::string prefix; std::string figureNumber; std::string suffix; - // The figure's continuation line. Absent when the figure has none. + // The figure's continuation line. Absent when the figure does not have one. std::optional extend; FigureData() : prefix{}, figureNumber{}, suffix{}, extend{} @@ -68,7 +68,7 @@ MXAPI_NOT_EQUALS_AND_VECTORS(FigureData); // The MusicXML element: an ordered list of figures plus optional parentheses and an // optional duration. Figured bass takes its position from the first regular note that follows it in -// score order, so it carries no explicit tick position of its own here; it is attached to the +// score order, so it does not carry a tick position of its own here; it is attached to the // DirectionData whose tickTimePosition locates it. class FiguredBassData { diff --git a/src/include/mx/api/GlissandoData.h b/src/include/mx/api/GlissandoData.h index c6d387b68..d2a21c20a 100644 --- a/src/include/mx/api/GlissandoData.h +++ b/src/include/mx/api/GlissandoData.h @@ -42,8 +42,8 @@ struct GlissandoStart LineData lineData; // line-type, dash-length, space-length // Playback timing (MusicXML's trill-sound attributes). These exist on only -- a - // glissando has no accelerate/beats/first-beat/last-beat -- and are left at their defaults - // when glissandoType is glissando. + // glissando does not have accelerate/beats/first-beat/last-beat -- and are left at their + // defaults when glissandoType is glissando. Bool accelerate; std::optional beats; std::optional firstBeat; diff --git a/src/include/mx/api/HarpPedalsData.h b/src/include/mx/api/HarpPedalsData.h index 01de07a0f..94e67fa01 100644 --- a/src/include/mx/api/HarpPedalsData.h +++ b/src/include/mx/api/HarpPedalsData.h @@ -49,7 +49,7 @@ MXAPI_NOT_EQUALS_AND_VECTORS(HarpPedalTuning); // harp's seven pedals. A complete diagram lists all seven in the standard order of the harp's // pedal mechanism (D, C, B on the left foot; E, F, G, A on the right); pedalTunings preserves // the order given. positionData captures default/relative x-y plus the horizontal and vertical -// alignment; its placement member is unused here because has no placement +// alignment; its placement member is unused here because does not have a placement // attribute (placement lives on the parent ). class HarpPedalsData { diff --git a/src/include/mx/api/Id.h b/src/include/mx/api/Id.h index 7fdca905d..03dc947e0 100644 --- a/src/include/mx/api/Id.h +++ b/src/include/mx/api/Id.h @@ -41,7 +41,7 @@ namespace api // of one name, so Id cannot check it. An id you invent must not collide with one already in the // score. // -// To write no id attribute at all, leave the std::optional that holds the Id empty. +// To leave out the id attribute, leave the std::optional that holds the Id empty. // // Copy, assign, compare, sort, and hash an Id the way you would any other value. Two Ids are equal // when their text is equal, and std::hash is specialized for Id, so an Id works as a key in both diff --git a/src/include/mx/api/ImageData.h b/src/include/mx/api/ImageData.h index f90588403..3082f306d 100644 --- a/src/include/mx/api/ImageData.h +++ b/src/include/mx/api/ImageData.h @@ -15,13 +15,13 @@ namespace mx { namespace api { -// A graphic placed in the music, MusicXML's direction: source locates the graphic file -// (a URL or file reference) and type gives its MIME type, e.g. "image/png". height and width, -// in tenths, scale the image; give one alone to scale proportionally. positionData captures +// A graphic placed in the music, MusicXML's direction: source locates the graphic file (a +// URL or file reference) and type gives its MIME type, e.g. "image/png". height and width, in +// tenths, scale the image; give one alone to scale proportionally. positionData captures // default/relative x-y plus the horizontal and vertical alignment of the image relative to its -// position point; an image's vertical alignment has no baseline variant, so a baseline value -// here is not written. positionData's placement member is unused because has no -// placement attribute (placement lives on the parent ). +// position point; an image's vertical alignment does not have a baseline variant, so a baseline +// value here is not written. positionData's placement member is unused because does not +// have a placement attribute (placement lives on the parent ). class ImageData { public: diff --git a/src/include/mx/api/KeyData.h b/src/include/mx/api/KeyData.h index fc7e77655..e521ab786 100644 --- a/src/include/mx/api/KeyData.h +++ b/src/include/mx/api/KeyData.h @@ -19,7 +19,7 @@ namespace api // it is A minor, with dorian it is D dorian. The mode does not change the accidentals that are // drawn; fifths alone determines those. // -// `none` is the mode of music that has no tonal center. It is how a keyless staff is written: +// `none` is the mode of music without a tonal center. It is how a keyless staff is written: // zero fifths with none. It is a mode value like any other, and it says nothing about // how the signature is drawn. // @@ -107,7 +107,7 @@ struct KeyData CancelLocation cancelLocation; // The mode of the key signature (major, minor, dorian, none, and so on). It is optional: the - // default, KeyMode::unspecified, states no mode and writes no element. KeyMode::none + // default, KeyMode::unspecified, does not state a mode or write a element. KeyMode::none // is a stated mode, and it writes none. KeyMode mode; diff --git a/src/include/mx/api/LyricData.h b/src/include/mx/api/LyricData.h index 43f7bba35..1c4a0934d 100644 --- a/src/include/mx/api/LyricData.h +++ b/src/include/mx/api/LyricData.h @@ -38,10 +38,10 @@ enum class LyricExtendType stop }; -// One additional syllabic/text run appended to a , joined to the run before it by -// . A syllable is occasionally written as more than one run - for example a source -// that embeds a non-breaking space or an undertie (‿) inside otherwise ordinary lyric text. -// The common case of a lyric with a single run needs none of this; see LyricData::continuations. +// One additional syllabic/text run appended to a , joined to the run before it by . +// A syllable is occasionally written as more than one run - for example a source that embeds a +// non-breaking space or an undertie (‿) inside otherwise ordinary lyric text. The common case of a +// lyric with a single run does not need any of this; see LyricData::continuations. class LyricTextSegment { public: diff --git a/src/include/mx/api/MarkData.h b/src/include/mx/api/MarkData.h index cc4f33fc5..11369fc07 100644 --- a/src/include/mx/api/MarkData.h +++ b/src/include/mx/api/MarkData.h @@ -171,16 +171,14 @@ enum class MarkType fermataHalfCurve, // half-curve (MusicXML 4.0) fermataCurlew, // curlew (MusicXML 4.0) - // sadly, in MusicXML we do not have an 'above' 'below' attribute for the fermata. - // instead it has an attribute 'type' that is either 'upright' or 'inverted'. - // it appears that implementations interpret 'upright' to mean the 'normal' or - // correct orientation and 'inverted' to mean that the fermata should be 'flipped' - // from its correct orientation. in other words the implementation has to figure - // out if the fermata is above or below the note on its own (using its own - // algorithm combined with any default-y and relative-y values). this mess is - // going to be beyond the scope of the MusicXML Class Library at this time. You - // cause the fermata 'type' attribute to be set using the enum values below, but - // it's not possible for this library to calculate the correct fermata glyph. + // MusicXML does not have an 'above' or 'below' attribute for the fermata. Instead it has a + // 'type' attribute that is either 'upright' or 'inverted'. Implementations appear to read + // 'upright' as the normal orientation and 'inverted' as flipped from it, so each program + // works out whether the fermata is above or below the note (using its own algorithm along + // with any default-y and relative-y values). + // + // The enum values below set the fermata 'type' attribute, but this library does not work out + // which fermata glyph is correct. fermataUpright, // fermataNormalUpright, // normal fermataAngledUpright, // angled diff --git a/src/include/mx/api/MarkDataChoice.h b/src/include/mx/api/MarkDataChoice.h index 8418e25bc..6e5045bec 100644 --- a/src/include/mx/api/MarkDataChoice.h +++ b/src/include/mx/api/MarkDataChoice.h @@ -32,15 +32,15 @@ struct TremoloMarkData // // A one-note tremolo states its slash count in the MarkType itself // (MarkType::tremoloSingleOne through MarkType::tremoloSingleFive), and an unmeasured tremolo - // has no slash count, so neither uses this field. + // does not have a slash count, so neither uses this field. std::optional tremoloMarks; // The canonical SMuFL glyph name to draw for an unmeasured tremolo (MarkType::tremoloUnmeasured), // from SMuFL's Tremolos range -- for example "pendereckiTremolo" or "unmeasuredTremolo". Leave // it absent to accept SMuFL's "buzzRoll", the glyph MusicXML assumes when no name is given. // - // MusicXML allows this on a one-note or two-note tremolo too, but defines no meaning for it - // there, and most applications ignore it. + // MusicXML allows this on a one-note or two-note tremolo too, but does not define a meaning + // for it there, and most applications ignore it. std::optional smufl; }; @@ -343,15 +343,15 @@ class MarkDataChoice // Builds a compound dynamic, unless the value is a single standard symbol, in which case the // result is a Kind::dynamic choice (auto-collapse). A lone other-dynamics symbol does not - // collapse -- it has no dedicated MusicXML element, so it stays a compound of one. + // collapse -- it does not have a dedicated MusicXML element, so it stays a compound of one. MarkDataChoice(CompoundDynamicsData value); MarkDataChoice(OtherNotationMarkData value); MarkDataChoice(HarmonicMarkData value); - // Builds an ornament payload, unless it has no accidentals, in which case the result is none: - // an ornament without accidentals needs no payload. + // Builds an ornament payload, unless it does not have accidentals, in which case the result is + // none: an ornament without accidentals does not need a payload. MarkDataChoice(OrnamentMarkData value); // Builds an accidental mark payload, unless every field is left at its default, in which case diff --git a/src/include/mx/api/NoteData.h b/src/include/mx/api/NoteData.h index f77eb716f..9973c3573 100644 --- a/src/include/mx/api/NoteData.h +++ b/src/include/mx/api/NoteData.h @@ -141,11 +141,11 @@ class NoteData // 's slash attribute. Only meaningful when isGrace is true. Bool graceSlash; - // A grace note has no written duration, so a player takes its time from a neighboring note or - // adds time of its own. The next three fields say how much. Leave them empty to let the player - // decide. They can be set alone or together, along with graceSlash, and like graceSlash they are - // ignored when isGrace is false. A steal time outside 0 to 100 is written as the nearer of 0 and - // 100, and a steal time that is not a number is not written. + // A grace note does not have a written duration, so a player takes its time from a neighboring + // note or adds time of its own. The next three fields say how much. Leave them empty to let the + // player decide. They can be set alone or together, along with graceSlash, and like graceSlash + // they are ignored when isGrace is false. A steal time outside 0 to 100 is written as the + // nearer of 0 and 100, and a steal time that is not a number is not written. // How much of the previous note's time this grace note takes, as a percentage from 0 to 100. // The previous note is cut short by that share and the grace note sounds in its place. @@ -171,11 +171,11 @@ class NoteData // The element's smufl attribute: the canonical SMuFL glyph name to draw for this // notehead, for example "noteheadSlashHorizontalEnds". It names a glyph directly, so a - // notation program can draw a notehead that MusicXML has no value for -- pair it with + // notation program can draw a notehead that MusicXML does not have a value for -- pair it with // Notehead::other -- or narrow one that MusicXML names only broadly, such as // Notehead::cluster. Note-name noteheads (the SMuFL ranges U+E150-U+E1AF and U+EEE0-U+EEFF) // are not written this way; MusicXML spells those out in . An empty string - // names no glyph and is written the same as leaving this empty. + // does not name a glyph and is written the same as leaving this empty. std::optional noteheadSmufl; // The element's parentheses attribute: whether the notehead is drawn inside diff --git a/src/include/mx/api/NoteRelationData.h b/src/include/mx/api/NoteRelationData.h index bfb02d871..608473b6c 100644 --- a/src/include/mx/api/NoteRelationData.h +++ b/src/include/mx/api/NoteRelationData.h @@ -59,7 +59,7 @@ struct MetronomeBeam // A tuplet drawn on a note figure inside a metric-relationship metronome mark: the // time-modification ratio -- actualNotes in the time of normalNotes, where a normal note is a // normalType note carrying normalDots dots -- plus how the bracket and numbers are shown. -// normalType unspecified means the ratio uses the figure's own note type. +// normalType unspecified means the ratio uses the figure's note type. struct MetronomeTuplet { int actualNotes; diff --git a/src/include/mx/api/OtherDirectionData.h b/src/include/mx/api/OtherDirectionData.h index 7eda94432..48f99d071 100644 --- a/src/include/mx/api/OtherDirectionData.h +++ b/src/include/mx/api/OtherDirectionData.h @@ -17,7 +17,7 @@ namespace mx { namespace api { -// A direction that has no dedicated MusicXML element, carried by the +// A direction that does not have a dedicated MusicXML element, carried by the // catch-all. The text is the direction's content; when the direction is best shown as a // standard SMuFL glyph, smufl names that glyph (a canonical SMuFL glyph name) and the text // serves as a fallback description. Set printObject to no for a direction that affects diff --git a/src/include/mx/api/OttavaData.h b/src/include/mx/api/OttavaData.h index a4628b509..1bd76c73a 100644 --- a/src/include/mx/api/OttavaData.h +++ b/src/include/mx/api/OttavaData.h @@ -34,7 +34,7 @@ class OttavaStart // Most users can ignore this; leave it false. It only controls whether an 8va/8vb line writes // the redundant size="8" attribute. The size (8, 15, or 22) follows from ottavaType, and 8 is // MusicXML's default, so a 15ma/15mb or 22ma/22mb line always writes its size while an 8va/8vb - // line omits it. When true, that redundant size="8" is written too; the flag has no effect on + // line omits it. When true, that redundant size="8" is written too; the flag does not affect // the larger lines, whose size is always written. It exists for round-trip fidelity - reading a // file sets it when the source spelled the attribute out. bool writeDefaultSize; @@ -51,7 +51,7 @@ class OttavaStop // Most users can ignore this; leave it unspecified. It only controls whether the stop end of an // octave-shift line repeats the line's size (8, 15, or 22). The size is not required on the - // stop and carries no new information. Omitting it is legal, but but some importers (MuseScore) + // stop and does not add information. Omitting it is legal, but some importers (MuseScore) // expect it. Therefore `mx::api` defaults to automatically writing the redundant stop size. // // For the sake of file round-trip fidelity, this will be set to 'no' if a the file being read diff --git a/src/include/mx/api/PageTextData.h b/src/include/mx/api/PageTextData.h index c9a55760c..f052179f2 100644 --- a/src/include/mx/api/PageTextData.h +++ b/src/include/mx/api/PageTextData.h @@ -65,8 +65,8 @@ class PageTextData // attributes on ``. HorizontalAlignment justify = HorizontalAlignment::unspecified; - // A shape drawn around the text. Enclosure::unspecified draws no enclosure; Enclosure::none - // states explicitly that there is none. + // A shape drawn around the text. Enclosure::unspecified does not draw an enclosure; + // Enclosure::none states explicitly that there is no enclosure. Enclosure enclosure = Enclosure::unspecified; // The stretches of text that follow `text` in the same credit, in order. Empty for a credit diff --git a/src/include/mx/api/PartData.h b/src/include/mx/api/PartData.h index c042812f4..6c0757754 100644 --- a/src/include/mx/api/PartData.h +++ b/src/include/mx/api/PartData.h @@ -23,24 +23,24 @@ struct MidiData std::string virtualLibrary; std::string virtualName; - // The name of the playback device or sound bank for this instrument — for example the name of a + // The name of the playback device or sound bank for this instrument, for example the name of a // MIDI output device, or a sound or bank label. This is often left empty. std::string device; // An optional port (or cable) number, from 1 to 16, that says which output the device uses. It - // has no value when the file doesn't specify one, and most files leave it unset — you only need + // is empty when the file doesn't specify one, and most files leave it unset; you only need // it when you are aiming at a particular MIDI port. You don't have to attach the device to this // part's instrument yourself; the library keeps them together automatically when the music is // written out. std::optional devicePort; // Most users can ignore this; leave it unspecified. It only controls whether the saved file - // spells out the little link that ties this part's playback device to its instrument. unspecified - // (the default) does the sensible thing on its own: it leaves that link out, because each part - // here has a single instrument and so the link isn't needed. yes/no force it to be written or left - // out. You never have to set this when you are creating music — when the library reads a file that - // did spell the link out, it sets this for you, so that saving the file brings it back the same way - // it came in. + // spells out the little link that ties this part's playback device to its instrument. + // unspecified (the default) does the sensible thing on its own: it leaves that link out, + // because each part here has a single instrument and so the link isn't needed. yes/no force it + // to be written or left out. You never have to set this when you are creating music. When the + // library reads a file that did spell the link out, it sets this for you, so that saving the + // file brings it back the same way it came in. Bool writeDeviceId; std::string name; @@ -85,13 +85,13 @@ struct MidiData // MusicXML itself allows a single part to carry more than one instrument. The most common case is // a drum kit or percussion part, where the bass drum, snare, ride cymbal, and so on are each // treated as a separate instrument. You also see it when a part layers or doubles two sounds that -// play together — two sampled instruments sounding at once, or a single "Clarinet 1 & 2" line that +// play together: two sampled instruments sounding at once, or a single "Clarinet 1 & 2" line that // really holds two separate instruments. // // For now this library brings in only the first instrument of such a part. Any additional // instruments in the file are not available here and are not written back out. We're sorry for the // limitation: we looked for a way to offer several instruments per part without making the ordinary -// single-instrument case — which covers the vast majority of music — harder to understand and use, +// single-instrument case (which covers the vast majority of music) harder to understand and use, // and we didn't find one we were happy with, so it stays one instrument per part for the time // being. // diff --git a/src/include/mx/api/PedalLineData.h b/src/include/mx/api/PedalLineData.h index 40cce86df..d85fba7ea 100644 --- a/src/include/mx/api/PedalLineData.h +++ b/src/include/mx/api/PedalLineData.h @@ -56,11 +56,11 @@ struct PedalLineData PositionData positionData; // Which line this event belongs to when two pedal lines are held down at once; MusicXML calls - // it the number. It can be left unspecified, and usually should be: a lone line, and any run - // of lines that do not overlap, needs no number. A number is only needed to tell two lines + // it the number. It can be left unspecified, and usually should be: a lone line, and any run of + // lines that do not overlap, does not need a number. A number is only needed to tell two lines // apart when they overlap -- when a line opens before the one before it has closed, as a - // sostenuto line under a damper line does -- because otherwise a lift cannot be matched to - // the right downstroke. In non-ambiguous cases, leaving this unset is fine. + // sostenuto line under a damper line does -- because otherwise a lift cannot be matched to the + // right downstroke. In non-ambiguous cases, leaving this unset is fine. SpannerNumber number; // The element's id attribute (see Id.h). diff --git a/src/include/mx/api/PercussionData.h b/src/include/mx/api/PercussionData.h index a2ea4abb5..372ffc70e 100644 --- a/src/include/mx/api/PercussionData.h +++ b/src/include/mx/api/PercussionData.h @@ -465,19 +465,19 @@ class PercussionDataChoice MXAPI_NOT_EQUALS_AND_VECTORS(PercussionDataChoice); -// A percussion pictogram, MusicXML's element: a symbol for an unpitched instrument -// or the implement striking it, used in percussion parts and legends. Several PercussionData in -// one direction read as a group (for example a membrane pictogram followed by a beater). -// positionData captures default/relative x-y plus the horizontal and vertical alignment; its -// placement member is unused here because has no placement attribute (placement -// lives on the parent ). +// A percussion pictogram, MusicXML's element: a symbol for an unpitched instrument or +// the implement striking it, used in percussion parts and legends. Several PercussionData in one +// direction read as a group (for example a membrane pictogram followed by a beater). positionData +// captures default/relative x-y plus the horizontal and vertical alignment; its placement member is +// unused here because does not have a placement attribute (placement lives on the +// parent ). class PercussionData { public: PercussionDataChoice choice; - // A shape drawn around the pictogram. Enclosure::unspecified draws no enclosure; - // Enclosure::none states explicitly that there is none. + // A shape drawn around the pictogram. Enclosure::unspecified does not draw an enclosure; + // Enclosure::none states explicitly that there is no enclosure. Enclosure enclosure; PositionData positionData; diff --git a/src/include/mx/api/RehearsalData.h b/src/include/mx/api/RehearsalData.h index e299c6962..a1ae02108 100644 --- a/src/include/mx/api/RehearsalData.h +++ b/src/include/mx/api/RehearsalData.h @@ -26,8 +26,8 @@ class RehearsalData ColorData colorData; FontData fontData; - // A shape drawn around the text. Enclosure::unspecified draws no enclosure; Enclosure::none - // states explicitly that there is none. + // A shape drawn around the text. Enclosure::unspecified does not draw an enclosure; + // Enclosure::none states explicitly that there is no enclosure. Enclosure enclosure; // The `justify` attribute of ``: how the mark's lines sit relative to each other diff --git a/src/include/mx/api/Result.h b/src/include/mx/api/Result.h index baf1a33aa..1ed07832f 100644 --- a/src/include/mx/api/Result.h +++ b/src/include/mx/api/Result.h @@ -18,8 +18,8 @@ namespace api // The mx::api error vocabulary. mx::api owns its own codes: the core-boundary // failures are mirrored (public headers never -// include private mx::core headers), and the api adds the codes core has no -// business knowing. No exceptions escape the MusicXml boundary. +// include private mx::core headers), and the api adds the codes that are not +// core's business. No exceptions escape the MusicXml boundary. enum class ResultCode { ioError, // file open/read/write failure (api-level) diff --git a/src/include/mx/api/ScoreData.h b/src/include/mx/api/ScoreData.h index 38ddb1308..ec351de56 100644 --- a/src/include/mx/api/ScoreData.h +++ b/src/include/mx/api/ScoreData.h @@ -115,9 +115,9 @@ class ScoreData // entry with no role does the same for composer when no one is credited as composer. std::vector additionalCreators; - // The score's rights statement, usually its copyright notice. When a score has several, this - // is the first one, whatever it covers. Leave it empty when the score has none. MusicXML calls - // this . + // The score's rights statement, usually its copyright notice. When a score has several, this is + // the first one, whatever it covers. Leave it empty when the score does not have one. MusicXML + // calls this . std::string copyright; // What kind of statement `copyright` is, or what it covers: "copyright" (the default), or for @@ -155,7 +155,7 @@ class ScoreData /// reports any other transposition as a diagnostic, without changing it. /// /// A score that has any ForPartData is always written as a concert score, so you only need to - /// set this for a concert score that has no transposing instruments. + /// set this for a concert score without transposing instruments. bool concertScore; std::vector parts; @@ -164,8 +164,8 @@ class ScoreData /// The score's time resolution: how many `tickTimePosition`/duration ticks make up one /// quarter note. Corresponds to the `` MusicXML element. Choose a value with /// enough factors for the durations you need (the default accommodates tuplets through - /// septuplets). Zero means the score has no time resolution at all — no durations exist - /// anywhere — and no `` is written. + /// septuplets). Zero means the score does not have a time resolution: no durations exist + /// anywhere, and no `` is written. int ticksPerQuarter; /// Specifies page breaks, system breaks, and changes to system and page layout. Global/default page and diff --git a/src/include/mx/api/SegnoData.h b/src/include/mx/api/SegnoData.h index 880932d9c..bdbd78a8c 100644 --- a/src/include/mx/api/SegnoData.h +++ b/src/include/mx/api/SegnoData.h @@ -19,8 +19,8 @@ namespace api { // The MusicXML element carries the empty-print-object-style-align attribute group plus the // smufl glyph-name and optional-unique-id. positionData captures default/relative x-y plus the -// horizontal and vertical alignment; its placement member is unused here because has no -// placement attribute (placement lives on the parent ). +// horizontal and vertical alignment; its placement member is unused here because does not +// have a placement attribute (placement lives on the parent ). class SegnoData { public: diff --git a/src/include/mx/api/StaffDivideData.h b/src/include/mx/api/StaffDivideData.h index fb5644e16..119e45602 100644 --- a/src/include/mx/api/StaffDivideData.h +++ b/src/include/mx/api/StaffDivideData.h @@ -25,11 +25,11 @@ enum class StaffDivideType upDown }; -// A staff-divide arrow, MusicXML's element (MusicXML 3.1): the arrow symbol used -// in condensed conductor scores where one staff carries two players, showing whether the music +// A staff-divide arrow, MusicXML's element (MusicXML 3.1): the arrow symbol used in +// condensed conductor scores where one staff carries two players, showing whether the music // continues on the staff below (down), above (up), or both (up-down). positionData captures -// default/relative x-y plus the horizontal and vertical alignment; its placement member is -// unused here because has no placement attribute (placement lives on the parent +// default/relative x-y plus the horizontal and vertical alignment; its placement member is unused +// here because does not have a placement attribute (placement lives on the parent // ). class StaffDivideData { diff --git a/src/include/mx/api/StringMuteData.h b/src/include/mx/api/StringMuteData.h index 5c2414867..e2ee3781b 100644 --- a/src/include/mx/api/StringMuteData.h +++ b/src/include/mx/api/StringMuteData.h @@ -27,7 +27,7 @@ enum class StringMuteType // A string mute change, MusicXML's element: the "mute on" or "mute off" symbol // telling string players to apply or remove their mutes. positionData captures default/relative // x-y plus the horizontal and vertical alignment; its placement member is unused here because -// has no placement attribute (placement lives on the parent ). +// does not have a placement attribute (placement lives on the parent ). class StringMuteData { public: diff --git a/src/include/mx/api/SymbolData.h b/src/include/mx/api/SymbolData.h index ad9d65eb0..0d7496f28 100644 --- a/src/include/mx/api/SymbolData.h +++ b/src/include/mx/api/SymbolData.h @@ -32,8 +32,8 @@ class SymbolData FontData fontData; std::optional color; - // A shape drawn around the symbol. Enclosure::unspecified draws no enclosure; Enclosure::none - // states explicitly that there is none. + // A shape drawn around the symbol. Enclosure::unspecified does not draw an enclosure; + // Enclosure::none states explicitly that there is no enclosure. Enclosure enclosure; // The `justify` attribute of ``. `unspecified` means the attribute is absent. Distinct diff --git a/src/include/mx/api/TimeChoice.h b/src/include/mx/api/TimeChoice.h index e1f742dcd..c8687174e 100644 --- a/src/include/mx/api/TimeChoice.h +++ b/src/include/mx/api/TimeChoice.h @@ -61,7 +61,7 @@ class TimeChoice Bool display{Bool::unspecified}; // The