From 0fd932f6330679f17c2be6083eb16be3cc8e2d1d Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Wed, 16 Sep 2026 10:39:38 +0200 Subject: [PATCH 1/2] feat: report non-fatal translation diagnostics (#432) --- .agents/skills/mx-api-doctrine/SKILL.md | 3 +- src/include/mx/api/Diagnostics.h | 64 ++++++ src/include/mx/api/MusicXml.h | 14 +- src/private/mx/api/Diagnostics.cpp | 67 ++++++ src/private/mx/api/LocationFormatting.cpp | 49 +++++ src/private/mx/api/LocationFormatting.h | 17 ++ src/private/mx/api/MusicXml.cpp | 26 ++- src/private/mx/api/Result.cpp | 40 +--- src/private/mx/examples/Read.cpp | 6 +- src/private/mx/impl/DiagnosticsContext.h | 50 +++++ src/private/mx/impl/MeasureReader.cpp | 26 ++- src/private/mx/impl/MeasureReader.h | 4 +- src/private/mx/impl/PartReader.cpp | 27 ++- src/private/mx/impl/PartReader.h | 5 +- src/private/mx/impl/ScoreReader.cpp | 6 +- src/private/mx/impl/ScoreReader.h | 4 +- src/private/mx/impl/ScoreWriter.cpp | 8 +- src/private/mx/impl/ScoreWriter.h | 4 +- src/private/mx/impl/SpannerResolver.cpp | 65 +++++- src/private/mx/impl/SpannerResolver.h | 13 +- src/private/mxtest/api/DiagnosticsTest.cpp | 242 +++++++++++++++++++++ 21 files changed, 640 insertions(+), 100 deletions(-) create mode 100644 src/include/mx/api/Diagnostics.h create mode 100644 src/private/mx/api/Diagnostics.cpp create mode 100644 src/private/mx/api/LocationFormatting.cpp create mode 100644 src/private/mx/api/LocationFormatting.h create mode 100644 src/private/mx/impl/DiagnosticsContext.h create mode 100644 src/private/mxtest/api/DiagnosticsTest.cpp diff --git a/.agents/skills/mx-api-doctrine/SKILL.md b/.agents/skills/mx-api-doctrine/SKILL.md index 4df0e675f..b99339bac 100644 --- a/.agents/skills/mx-api-doctrine/SKILL.md +++ b/.agents/skills/mx-api-doctrine/SKILL.md @@ -42,7 +42,8 @@ Responses to wrong api usage, in order of preference: 2. Defined fallback: document a harmless result and return it. A wrong-kind choice accessor returns a default-constructed copy; the writer drops the half of an encoding that is meaningless for the note it is on (a tie on a silent cue note is written as `` - notation only, never as a sound-level ``). No signal to the caller. + notation only, never as a sound-level ``). Report an important adjustment through an + optional `Diagnostics` collector; ignoring diagnostics must leave a safe, usable value. 3. `Result` (`Result.h`): the error channel of last resort. It exists for the `MusicXml` I/O boundary, where failure is real (unreadable file, unparseable XML). Do not spread it into the data model. diff --git a/src/include/mx/api/Diagnostics.h b/src/include/mx/api/Diagnostics.h new file mode 100644 index 000000000..368e048a4 --- /dev/null +++ b/src/include/mx/api/Diagnostics.h @@ -0,0 +1,64 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#pragma once + +#include "mx/api/Result.h" + +#include +#include +#include +#include + +namespace mx +{ +namespace api +{ +// How a successful translation affected the resulting score or document. +enum class Severity +{ + info, // noteworthy normalization without meaningful loss + warning, // adjusted data; the result remains usable + error // omitted data; the operation still produced a value +}; + +// The recoverable decision reported by a diagnostic. +enum class DiagnosticCode +{ + valueAdjusted, + unmatchedSpanner +}; + +// A non-fatal problem noticed while producing a value. +struct Diagnostic +{ + Severity severity = Severity::warning; + DiagnosticCode code = DiagnosticCode::valueAdjusted; + Location location; + std::string message; +}; + +using DiagnosticHandler = std::function; + +// Collects diagnostics from one or more translations. A handler, when supplied, +// runs as each diagnostic is added. +class Diagnostics +{ + public: + Diagnostics() = default; + explicit Diagnostics(DiagnosticHandler handler); + + void add(Diagnostic diagnostic); + std::span all() const noexcept; + bool hasSeverityOrWorse(Severity threshold) const noexcept; + + private: + std::vector myDiagnostics; + DiagnosticHandler myHandler; +}; + +// Writes a diagnostic on one line for logs and test output. +std::string formatDiagnostic(const Diagnostic &diagnostic); +} // namespace api +} // namespace mx diff --git a/src/include/mx/api/MusicXml.h b/src/include/mx/api/MusicXml.h index 45af2d11b..45c088f82 100644 --- a/src/include/mx/api/MusicXml.h +++ b/src/include/mx/api/MusicXml.h @@ -4,6 +4,7 @@ #pragma once +#include "mx/api/Diagnostics.h" #include "mx/api/Result.h" #include "mx/api/ScoreData.h" @@ -66,24 +67,29 @@ class MusicXml class Impl; std::unique_ptr myImpl; - friend Result getScore(const MusicXml &document); - friend Result fromScore(const ScoreData &score); + friend Result getScore(const MusicXml &document, Diagnostics &diagnostics); + friend Result fromScore(const ScoreData &score, Diagnostics &diagnostics); }; // Reads the score out of the document. The document stays alive and can be -// read again or written out. +// read again or written out. Pass Diagnostics to observe adjustments made +// while translating the document. Result getScore(const MusicXml &document); +Result getScore(const MusicXml &document, Diagnostics &diagnostics); // Reads the score out of the document and consumes it: the underlying tree // is freed when this function returns rather than when your MusicXml binding // goes out of scope. Pass the document with std::move, or hand over the // Result's value directly. Result intoScore(MusicXml document); +Result intoScore(MusicXml document, Diagnostics &diagnostics); // Authors a new document from ScoreData. Fails with an error result when the // ScoreData describes something the core model will not represent (e.g. more -// than 8 beams) rather than silently dropping data. +// than 8 beams) rather than silently dropping data. Pass Diagnostics to +// observe recoverable authoring decisions. Result fromScore(const ScoreData &score); +Result fromScore(const ScoreData &score, Diagnostics &diagnostics); } // namespace api } // namespace mx diff --git a/src/private/mx/api/Diagnostics.cpp b/src/private/mx/api/Diagnostics.cpp new file mode 100644 index 000000000..0ae696389 --- /dev/null +++ b/src/private/mx/api/Diagnostics.cpp @@ -0,0 +1,67 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mx/api/Diagnostics.h" + +#include "mx/api/LocationFormatting.h" + +#include + +namespace mx +{ +namespace api +{ +Diagnostics::Diagnostics(DiagnosticHandler handler) : myHandler{std::move(handler)} +{ +} + +void Diagnostics::add(Diagnostic diagnostic) +{ + myDiagnostics.push_back(std::move(diagnostic)); + if (myHandler) + { + myHandler(myDiagnostics.back()); + } +} + +std::span Diagnostics::all() const noexcept +{ + return myDiagnostics; +} + +bool Diagnostics::hasSeverityOrWorse(Severity threshold) const noexcept +{ + for (const auto &diagnostic : myDiagnostics) + { + if (diagnostic.severity >= threshold) + { + return true; + } + } + return false; +} + +std::string formatDiagnostic(const Diagnostic &diagnostic) +{ + const char *severityName = "warning"; + switch (diagnostic.severity) + { + case Severity::info: + severityName = "info"; + break; + case Severity::warning: + severityName = "warning"; + break; + case Severity::error: + severityName = "error"; + break; + } + + std::string text{"mx: "}; + text += severityName; + appendFormattedLocationAndMessage(text, diagnostic.location, diagnostic.message); + return text; +} +} // namespace api +} // namespace mx diff --git a/src/private/mx/api/LocationFormatting.cpp b/src/private/mx/api/LocationFormatting.cpp new file mode 100644 index 000000000..9ec425307 --- /dev/null +++ b/src/private/mx/api/LocationFormatting.cpp @@ -0,0 +1,49 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mx/api/LocationFormatting.h" + +namespace mx +{ +namespace api +{ +void appendFormattedLocationAndMessage(std::string &text, const Location &location, const std::string &message) +{ + std::string where; + const auto appendWhere = [&where](const char *name, long long value) { + if (value < 0) + { + return; + } + if (!where.empty()) + { + where += ' '; + } + where += name; + where += '='; + where += std::to_string(value); + }; + + where += location.xmlPath; + appendWhere("part", location.partIndex); + appendWhere("measure", location.measureIndex); + appendWhere("staff", location.staffIndex); + appendWhere("voice", location.voiceIndex); + appendWhere("tick", location.tickTimePosition); + appendWhere("offset", location.byteOffset); + + if (!where.empty()) + { + text += " at "; + text += where; + } + + if (!message.empty()) + { + text += ": "; + text += message; + } +} +} // namespace api +} // namespace mx diff --git a/src/private/mx/api/LocationFormatting.h b/src/private/mx/api/LocationFormatting.h new file mode 100644 index 000000000..b130f4ba3 --- /dev/null +++ b/src/private/mx/api/LocationFormatting.h @@ -0,0 +1,17 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#pragma once + +#include "mx/api/Result.h" + +#include + +namespace mx +{ +namespace api +{ +void appendFormattedLocationAndMessage(std::string &text, const Location &location, const std::string &message); +} // namespace api +} // namespace mx diff --git a/src/private/mx/api/MusicXml.cpp b/src/private/mx/api/MusicXml.cpp index 90682bb6f..d230b4bec 100644 --- a/src/private/mx/api/MusicXml.cpp +++ b/src/private/mx/api/MusicXml.cpp @@ -312,10 +312,16 @@ const core::Document &MusicXml::getCoreDocument() const } Result fromScore(const ScoreData &score) +{ + Diagnostics diagnostics; + return fromScore(score, diagnostics); +} + +Result fromScore(const ScoreData &score, Diagnostics &diagnostics) { try { - impl::ScoreWriter writer{score}; + impl::ScoreWriter writer{score, impl::DiagnosticsContext{diagnostics}}; core::ScorePartwise scorePartwise = writer.getScorePartwise(); if (score.musicXmlType == "timewise") @@ -346,6 +352,12 @@ Result fromScore(const ScoreData &score) } Result getScore(const MusicXml &document) +{ + Diagnostics diagnostics; + return getScore(document, diagnostics); +} + +Result getScore(const MusicXml &document, Diagnostics &diagnostics) { try { @@ -356,13 +368,13 @@ Result getScore(const MusicXml &document) if (coreDocument.isScoreTimewise()) { const core::ScorePartwise scorePartwise = impl::timewisePartwise(coreDocument.asScoreTimewise()); - impl::ScoreReader reader{scorePartwise}; + impl::ScoreReader reader{scorePartwise, impl::DiagnosticsContext{diagnostics}}; auto score = reader.getScoreData(); score.musicXmlType = "timewise"; return score; } - impl::ScoreReader reader{coreDocument.asScorePartwise()}; + impl::ScoreReader reader{coreDocument.asScorePartwise(), impl::DiagnosticsContext{diagnostics}}; return reader.getScoreData(); } catch (const std::bad_alloc &) @@ -380,10 +392,16 @@ Result getScore(const MusicXml &document) } Result intoScore(MusicXml document) +{ + Diagnostics diagnostics; + return intoScore(std::move(document), diagnostics); +} + +Result intoScore(MusicXml document, Diagnostics &diagnostics) { // the parameter owns the document; its destructor frees the underlying // tree when this function returns - return getScore(document); + return getScore(document, diagnostics); } } // namespace api } // namespace mx diff --git a/src/private/mx/api/Result.cpp b/src/private/mx/api/Result.cpp index e6a4b8864..d1af5d9a6 100644 --- a/src/private/mx/api/Result.cpp +++ b/src/private/mx/api/Result.cpp @@ -4,6 +4,8 @@ #include "mx/api/Result.h" +#include "mx/api/LocationFormatting.h" + namespace mx { namespace api @@ -53,43 +55,7 @@ std::string formatError(const ApiError &error) std::string text{"mx: "}; text += codeName; - - // the place in the document or the score, if known - std::string where; - const auto appendWhere = [&where](const char *name, long long value) { - if (value < 0) - { - return; - } - if (!where.empty()) - { - where += ' '; - } - where += name; - where += '='; - where += std::to_string(value); - }; - - where += error.location.xmlPath; - appendWhere("part", error.location.partIndex); - appendWhere("measure", error.location.measureIndex); - appendWhere("staff", error.location.staffIndex); - appendWhere("voice", error.location.voiceIndex); - appendWhere("tick", error.location.tickTimePosition); - appendWhere("offset", error.location.byteOffset); - - if (!where.empty()) - { - text += " at "; - text += where; - } - - if (!error.message.empty()) - { - text += ": "; - text += error.message; - } - + appendFormattedLocationAndMessage(text, error.location, error.message); return text; } } // namespace api diff --git a/src/private/mx/examples/Read.cpp b/src/private/mx/examples/Read.cpp index 15ffbfed1..6ccc76e3b 100644 --- a/src/private/mx/examples/Read.cpp +++ b/src/private/mx/examples/Read.cpp @@ -63,9 +63,13 @@ int main(int argc, const char *argv[]) return MX_IS_A_FAILURE; } + // collect any non-fatal adjustments made while reading. A handler may + // also display each one as it is found. + Diagnostics diagnostics{[](const Diagnostic &diagnostic) { std::cerr << formatDiagnostic(diagnostic) << '\n'; }}; + // take the score out of the document. intoScore also consumes the // document, so its memory is freed as the function returns - const auto scoreResult = intoScore(std::move(docResult).value()); + const auto scoreResult = intoScore(std::move(docResult).value(), diagnostics); if (!scoreResult.ok()) { return MX_IS_A_FAILURE; diff --git a/src/private/mx/impl/DiagnosticsContext.h b/src/private/mx/impl/DiagnosticsContext.h new file mode 100644 index 000000000..918c904b1 --- /dev/null +++ b/src/private/mx/impl/DiagnosticsContext.h @@ -0,0 +1,50 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#pragma once + +#include "mx/api/Diagnostics.h" +#include "mx/impl/MeasureCursor.h" + +#include +#include + +namespace mx +{ +namespace impl +{ +class DiagnosticsContext +{ + public: + DiagnosticsContext() = default; + + explicit DiagnosticsContext(api::Diagnostics &diagnostics) : myDiagnostics{&diagnostics} + { + } + + void report(api::Severity severity, api::DiagnosticCode code, api::Location location, std::string message) const + { + if (myDiagnostics) + { + myDiagnostics->add(api::Diagnostic{severity, code, std::move(location), std::move(message)}); + } + } + + void report(api::Severity severity, api::DiagnosticCode code, const MeasureCursor &cursor, + std::string message) const + { + api::Location location; + location.partIndex = cursor.partIndex; + location.measureIndex = cursor.measureIndex; + location.staffIndex = cursor.staffIndex; + location.voiceIndex = cursor.voiceIndex; + location.tickTimePosition = cursor.tickTimePosition; + report(severity, code, std::move(location), std::move(message)); + } + + private: + api::Diagnostics *myDiagnostics = nullptr; +}; +} // namespace impl +} // namespace mx diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index 27d0bfa79..3b4adc0c1 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -69,9 +69,7 @@ namespace mx { namespace impl { -namespace -{ -api::FigureData parseFigure(const core::Figure &figure) +api::FigureData measureReaderParseFigure(const core::Figure &figure) { api::FigureData figureData; @@ -93,8 +91,8 @@ api::FigureData parseFigure(const core::Figure &figure) return figureData; } -int getFiguredBassStaffIndex(const MeasureCursor &cursor, const api::MeasureData &measure, - const core::Note *nextNotePtr) +int measureReaderFiguredBassStaffIndex(const MeasureCursor &cursor, const api::MeasureData &measure, + const core::Note *nextNotePtr) { auto staffIndex = cursor.staffIndex; @@ -110,12 +108,11 @@ int getFiguredBassStaffIndex(const MeasureCursor &cursor, const api::MeasureData return staffIndex; } -} // namespace MeasureReader::MeasureReader(const core::PartwiseMeasure &inPartwiseMeasureRef, const MeasureCursor &cursor, - const MeasureCursor &previousMeasureCursor) - : myMutex{}, myPartwiseMeasure{inPartwiseMeasureRef}, myConverter{}, myOutMeasureData{}, myCurrentCursor{cursor}, - myPreviousMeasureCursor{previousMeasureCursor}, myHistory{}, myCrossStaffHomes{}, + const MeasureCursor &previousMeasureCursor, DiagnosticsContext diagnostics) + : myMutex{}, myPartwiseMeasure{inPartwiseMeasureRef}, myConverter{}, myDiagnostics{diagnostics}, myOutMeasureData{}, + myCurrentCursor{cursor}, myPreviousMeasureCursor{previousMeasureCursor}, myHistory{}, myCrossStaffHomes{}, myPreviousNoteBucketStaffIndex{-1} { HistoryRecord initialCursorRecord; @@ -253,6 +250,13 @@ void MeasureReader::parseTimeSignature() const // clamp an out-of-range staff number to "all staves", mirroring the keys pattern if (staffIndex != api::INDEX_UNSPECIFIED && staffIndex > myCurrentCursor.getNumStaves() - 1) { + api::Location location; + location.partIndex = myCurrentCursor.partIndex; + location.measureIndex = myCurrentCursor.measureIndex; + location.tickTimePosition = myCurrentCursor.tickTimePosition; + myDiagnostics.report(api::Severity::warning, api::DiagnosticCode::valueAdjusted, std::move(location), + "time signature staff number " + std::to_string(staffIndex + 1) + + " is out of range; applying it to all staves"); staffIndex = api::INDEX_UNSPECIFIED; } @@ -824,7 +828,7 @@ void MeasureReader::parseFiguredBass(const core::FiguredBass &inMxFiguredBass, c for (const auto &figure : inMxFiguredBass.figure()) { - figuredBass.figures.emplace_back(parseFigure(figure)); + figuredBass.figures.emplace_back(measureReaderParseFigure(figure)); } if (inMxFiguredBass.parentheses().has_value()) @@ -852,7 +856,7 @@ void MeasureReader::parseFiguredBass(const core::FiguredBass &inMxFiguredBass, c direction.figuredBasses.emplace_back(std::move(figuredBass)); - const auto staffIndex = getFiguredBassStaffIndex(myCurrentCursor, myOutMeasureData, nextNotePtr); + const auto staffIndex = measureReaderFiguredBassStaffIndex(myCurrentCursor, myOutMeasureData, nextNotePtr); myOutMeasureData.staves.at(static_cast(staffIndex)).directions.emplace_back(std::move(direction)); } diff --git a/src/private/mx/impl/MeasureReader.h b/src/private/mx/impl/MeasureReader.h index 5edbc05d7..4cadd37ce 100644 --- a/src/private/mx/impl/MeasureReader.h +++ b/src/private/mx/impl/MeasureReader.h @@ -6,6 +6,7 @@ #include "mx/api/MeasureData.h" #include "mx/impl/Converter.h" +#include "mx/impl/DiagnosticsContext.h" #include "mx/impl/MeasureCursor.h" #include @@ -40,7 +41,7 @@ class MeasureReader { public: MeasureReader(const core::PartwiseMeasure &inPartwiseMeasureRef, const MeasureCursor &cursor, - const MeasureCursor &previousMeasureCursor); + const MeasureCursor &previousMeasureCursor, DiagnosticsContext diagnostics = {}); std::pair> getMeasureData() const; impl::MeasureCursor getCursor() const; @@ -49,6 +50,7 @@ class MeasureReader mutable std::mutex myMutex; const core::PartwiseMeasure &myPartwiseMeasure; const Converter myConverter; + DiagnosticsContext myDiagnostics; mutable api::MeasureData myOutMeasureData; mutable MeasureCursor myCurrentCursor; diff --git a/src/private/mx/impl/PartReader.cpp b/src/private/mx/impl/PartReader.cpp index 74f5665eb..600b55a03 100644 --- a/src/private/mx/impl/PartReader.cpp +++ b/src/private/mx/impl/PartReader.cpp @@ -40,11 +40,9 @@ namespace mx { namespace impl { -namespace -{ // True when a / carries any of the formatting // attributes that MusicXML 2.0 deprecated in favor of the *-display elements. -bool nameHasDeprecatedFormatting(const core::PartName &n) +bool partReaderNameHasDeprecatedFormatting(const core::PartName &n) { return n.fontFamily().has_value() || n.fontStyle().has_value() || n.fontSize().has_value() || n.fontWeight().has_value() || n.color().has_value() || n.defaultX().has_value() || @@ -57,15 +55,15 @@ bool nameHasDeprecatedFormatting(const core::PartName &n) // a present *-display element is canonical and wins; otherwise any deprecated // formatting on the name element itself is migrated into the display model so it // is re-emitted at the modern location. -void readNameDisplay(const core::PartName &nameElement, const std::optional &display, - std::string &outText, api::PrintData &outPrintData, api::PositionData &outPositionData) +void partReaderReadNameDisplay(const core::PartName &nameElement, const std::optional &display, + std::string &outText, api::PrintData &outPrintData, api::PositionData &outPositionData) { if (display.has_value()) { outText = extractDisplayText(*display); extractDisplayFormatting(*display, outPrintData, outPositionData); } - else if (nameHasDeprecatedFormatting(nameElement)) + else if (partReaderNameHasDeprecatedFormatting(nameElement)) { outText = nameElement.value(); outPrintData = getPrintData(nameElement); @@ -74,13 +72,13 @@ void readNameDisplay(const core::PartName &nameElement, const std::optional @@ -29,7 +30,8 @@ class PartReader { public: PartReader(const core::ScorePart &inScorePart, const core::PartwisePart &inPartwisePartRef, - int globalTicksPerMeasure, const core::ScorePartwise &inScore, int inDivisionsValue); + int globalTicksPerMeasure, const core::ScorePartwise &inScore, int inDivisionsValue, + DiagnosticsContext diagnostics = {}); api::PartData getPartData(); impl::MeasureCursor getCursor() const; @@ -43,6 +45,7 @@ class PartReader const core::ScorePartwise &myScore; int myPartIndex; const int myConstructedDivisionsValue; + DiagnosticsContext myDiagnostics; MeasureCursor myCurrentCursor; MeasureCursor myPreviousCursor; diff --git a/src/private/mx/impl/ScoreReader.cpp b/src/private/mx/impl/ScoreReader.cpp index cbc10de32..59fcedbbf 100644 --- a/src/private/mx/impl/ScoreReader.cpp +++ b/src/private/mx/impl/ScoreReader.cpp @@ -44,9 +44,9 @@ namespace mx { namespace impl { -ScoreReader::ScoreReader(const core::ScorePartwise &inScorePartwise) +ScoreReader::ScoreReader(const core::ScorePartwise &inScorePartwise, DiagnosticsContext diagnostics) : myScorePartwise{inScorePartwise}, myPartSet{inScorePartwise.part()}, myHeaderGroup{inScorePartwise.scoreHeader()}, - myMutex{}, myOutScoreData{}, myPartGroupStack{} + myDiagnostics{diagnostics}, myMutex{}, myOutScoreData{}, myPartGroupStack{} { } @@ -268,7 +268,7 @@ api::ScoreData ScoreReader::getScoreData() const for (const auto &reconciledPart : partMap) { PartReader reader{*reconciledPart.first, *reconciledPart.second, myOutScoreData.ticksPerQuarter, - myScorePartwise, divisionsValue}; + myScorePartwise, divisionsValue, myDiagnostics}; myOutScoreData.parts.emplace_back(reader.getPartData()); const auto cursorReturn = reader.getCursor(); divisionsValue = cursorReturn.ticksPerQuarter; diff --git a/src/private/mx/impl/ScoreReader.h b/src/private/mx/impl/ScoreReader.h index 0733cb012..0059fec12 100644 --- a/src/private/mx/impl/ScoreReader.h +++ b/src/private/mx/impl/ScoreReader.h @@ -6,6 +6,7 @@ #include "mx/api/ScoreData.h" #include "mx/core/generated/ScorePartwise.h" +#include "mx/impl/DiagnosticsContext.h" #include #include @@ -20,7 +21,7 @@ namespace impl class ScoreReader { public: - ScoreReader(const core::ScorePartwise &inScorePartwise); + ScoreReader(const core::ScorePartwise &inScorePartwise, DiagnosticsContext diagnostics = {}); api::ScoreData getScoreData() const; @@ -28,6 +29,7 @@ class ScoreReader const core::ScorePartwise &myScorePartwise; std::span myPartSet; const core::ScoreHeaderGroup &myHeaderGroup; + DiagnosticsContext myDiagnostics; private: mutable std::mutex myMutex; diff --git a/src/private/mx/impl/ScoreWriter.cpp b/src/private/mx/impl/ScoreWriter.cpp index e4ef84e9c..ea9f2f2c3 100644 --- a/src/private/mx/impl/ScoreWriter.cpp +++ b/src/private/mx/impl/ScoreWriter.cpp @@ -31,16 +31,18 @@ namespace mx { namespace impl { -ScoreWriter::ScoreWriter(const api::ScoreData &inScoreData) - : myScoreData{inScoreData}, mySpannerResolver{}, myMutex{}, myOutScorePartwise{} +ScoreWriter::ScoreWriter(const api::ScoreData &inScoreData, DiagnosticsContext diagnostics) + : myScoreData{inScoreData}, myDiagnostics{diagnostics}, mySpannerResolver{}, myMutex{}, myOutScorePartwise{} { myScoreData.sort(); // Resolve after sort() so the resolved spanner object addresses are the // ones the measure/note writers will visit. + int partIndex = 0; for (const auto &part : myScoreData.parts) { - mySpannerResolver.resolvePart(part); + mySpannerResolver.resolvePart(part, partIndex, myDiagnostics); + ++partIndex; } } diff --git a/src/private/mx/impl/ScoreWriter.h b/src/private/mx/impl/ScoreWriter.h index 5cb5e2d01..8efa9a9e1 100644 --- a/src/private/mx/impl/ScoreWriter.h +++ b/src/private/mx/impl/ScoreWriter.h @@ -10,6 +10,7 @@ #include "mx/core/generated/ScorePart.h" #include "mx/core/generated/ScorePartwise.h" #include "mx/impl/Cursor.h" +#include "mx/impl/DiagnosticsContext.h" #include "mx/impl/SpannerResolver.h" #include @@ -23,7 +24,7 @@ namespace impl class ScoreWriter { public: - ScoreWriter(const api::ScoreData &inScoreData); + ScoreWriter(const api::ScoreData &inScoreData, DiagnosticsContext diagnostics = {}); core::ScorePartwise getScorePartwise() const; @@ -47,6 +48,7 @@ class ScoreWriter private: api::ScoreData myScoreData; + DiagnosticsContext myDiagnostics; SpannerResolver mySpannerResolver; mutable std::mutex myMutex; mutable core::ScorePartwise myOutScorePartwise; diff --git a/src/private/mx/impl/SpannerResolver.cpp b/src/private/mx/impl/SpannerResolver.cpp index 2625e980a..a6253fc27 100644 --- a/src/private/mx/impl/SpannerResolver.cpp +++ b/src/private/mx/impl/SpannerResolver.cpp @@ -5,6 +5,7 @@ #include "mx/impl/SpannerResolver.h" #include "mx/api/GlissandoData.h" #include "mx/impl/OttavaFunctions.h" +#include "mx/impl/WriteRefusal.h" #include "mx/utility/Throw.h" #include @@ -52,6 +53,7 @@ struct SpannerNumberEvent // The note the event sits on, used to detect spanners that start and stop // on one note. Direction events do not belong to a note and carry nullptr. const void *noteTag; + api::Location location; }; // [first, last] positions (inclusive) during which a spanner is open in the @@ -75,6 +77,7 @@ struct SpannerOttavaEvent api::SpannerNumber number; api::OttavaType ottavaType; bool isStart; + api::Location location; }; // The bucket an endpoint pairs within, used for ottava sizes and for same-note span detection: @@ -103,9 +106,30 @@ static std::string spannerPairingKey(const api::SpannerNumber &inNumber) class SpannerEventCollector { public: + explicit SpannerEventCollector(int partIndex) : myLocation{} + { + myLocation.partIndex = partIndex; + } + + void setMeasure(int measureIndex) + { + myLocation.measureIndex = measureIndex; + } + + void setStaff(int staffIndex) + { + myLocation.staffIndex = staffIndex; + } + + void setVoice(int voiceIndex) + { + myLocation.voiceIndex = voiceIndex; + } + void addNote(const api::NoteData &inNote) { myCurrentNoteTag = &inNote; + myLocation.tickTimePosition = inNote.tickTimePosition; // NotationsWriter emits curve stops, then continues, then starts, and // skips curves whose type is neither tie nor slur. @@ -156,6 +180,8 @@ class SpannerEventCollector void addDirection(const api::DirectionData &inDirection) { myCurrentNoteTag = nullptr; + myLocation.voiceIndex = inDirection.voice; + myLocation.tickTimePosition = inDirection.tickTimePosition; for (const auto &choice : inDirection.directionTypes) { @@ -172,16 +198,16 @@ class SpannerEventCollector case api::DirectionChoice::Kind::ottavaStart: { const auto ottavaStart = choice.ottavaStart(); add(SpannerNumberClass::octaveShift, &choice, ottavaStart.spannerStart.number, true, false); - myOttavaEvents.push_back( - SpannerOttavaEvent{&choice, ottavaStart.spannerStart.number, ottavaStart.ottavaType, true}); + myOttavaEvents.push_back(SpannerOttavaEvent{&choice, ottavaStart.spannerStart.number, + ottavaStart.ottavaType, true, myLocation}); break; } case api::DirectionChoice::Kind::ottavaStop: { const auto ottavaStop = choice.ottavaStop(); add(SpannerNumberClass::octaveShift, &choice, ottavaStop.spannerStop.number, false, true); - myOttavaEvents.push_back( - SpannerOttavaEvent{&choice, ottavaStop.spannerStop.number, api::OttavaType::unspecified, false}); + myOttavaEvents.push_back(SpannerOttavaEvent{&choice, ottavaStop.spannerStop.number, + api::OttavaType::unspecified, false, myLocation}); break; } @@ -222,7 +248,7 @@ class SpannerEventCollector bool inCloses) { myEvents[inClass].push_back( - SpannerNumberEvent{myPosition, inObject, inNumber, inOpens, inCloses, myCurrentNoteTag}); + SpannerNumberEvent{myPosition, inObject, inNumber, inOpens, inCloses, myCurrentNoteTag, myLocation}); ++myPosition; } @@ -249,6 +275,7 @@ class SpannerEventCollector int myPosition = 0; const void *myCurrentNoteTag = nullptr; + api::Location myLocation; std::map> myEvents; std::vector myOttavaEvents; }; @@ -322,6 +349,7 @@ static void spannerNumberAssignClass(const std::vector &inEv { SpannerNumberInterval interval; std::vector objects; + api::Location location; }; std::vector groups; @@ -335,7 +363,8 @@ static void spannerNumberAssignClass(const std::vector &inEv const auto found = groupIndexByIdentity.emplace(event.number.identity(), groups.size()); if (found.second) { - groups.push_back(SpannerNumberGroup{SpannerNumberInterval{event.position, event.position}, {}}); + groups.push_back( + SpannerNumberGroup{SpannerNumberInterval{event.position, event.position}, {}, event.location}); } auto &group = groups.at(found.first->second); group.interval.first = std::min(group.interval.first, event.position); @@ -368,8 +397,10 @@ static void spannerNumberAssignClass(const std::vector &inEv } if (chosen == 0) { - MX_THROW("more than 16 spanners of one type are open at the same point in the serialized " - "stream; MusicXML number attributes only range from 1 to 16"); + throw WriteRefusal{api::ApiError{ + api::ResultCode::tooManyElements, group.location, + "more than 16 spanners of one type are open at the same point; MusicXML number attributes only " + "range from 1 to 16"}}; } occupied[chosen].push_back(group.interval); for (const void *object : group.objects) @@ -384,7 +415,8 @@ static void spannerNumberAssignClass(const std::vector &inEv // and closes inside another ottava of the same bucket does not steal the outer line's start. A // stop with nothing open is left out of ioResolved and falls back to size 8 at write time. static void spannerResolveOttavaSizes(const std::vector &inEvents, - std::unordered_map &ioResolved) + std::unordered_map &ioResolved, + const DiagnosticsContext &diagnostics) { std::map> openStarts; @@ -398,6 +430,8 @@ static void spannerResolveOttavaSizes(const std::vector &inE } if (stack.empty()) { + diagnostics.report(api::Severity::warning, api::DiagnosticCode::unmatchedSpanner, event.location, + "octave-shift stop has no matching start; using size 8"); continue; } ioResolved[event.object] = ottavaTypeSize(stack.back()); @@ -453,16 +487,21 @@ static void spannerDetectSameNoteSpans(const std::vector &in } } -void SpannerResolver::resolvePart(const api::PartData &inPart) +void SpannerResolver::resolvePart(const api::PartData &inPart, int partIndex, DiagnosticsContext diagnostics) { - SpannerEventCollector collector; + SpannerEventCollector collector{partIndex}; + int measureIndex = 0; for (const auto &measure : inPart.measures) { + collector.setMeasure(measureIndex); + int staffIndex = 0; for (const auto &staff : measure.staves) { + collector.setStaff(staffIndex); for (const auto &voicePair : staff.voices) { + collector.setVoice(voicePair.first); for (const auto ¬e : voicePair.second.notes) { collector.addNote(note); @@ -472,7 +511,9 @@ void SpannerResolver::resolvePart(const api::PartData &inPart) { collector.addDirection(direction); } + ++staffIndex; } + ++measureIndex; } for (const auto &classAndEvents : collector.events()) @@ -487,7 +528,7 @@ void SpannerResolver::resolvePart(const api::PartData &inPart) } } - spannerResolveOttavaSizes(collector.ottavaEvents(), myOttavaStopSizes); + spannerResolveOttavaSizes(collector.ottavaEvents(), myOttavaStopSizes, diagnostics); } std::optional SpannerResolver::emittedNumber(const api::SpannerNumber &inNumber, const void *inObject) const diff --git a/src/private/mx/impl/SpannerResolver.h b/src/private/mx/impl/SpannerResolver.h index 0f7b74197..2255de5e9 100644 --- a/src/private/mx/impl/SpannerResolver.h +++ b/src/private/mx/impl/SpannerResolver.h @@ -5,6 +5,7 @@ #pragma once #include "mx/api/ScoreData.h" +#include "mx/impl/DiagnosticsContext.h" #include #include @@ -53,8 +54,8 @@ namespace impl // here and no number is ever emitted for one. // // If more than 16 spanners of one class are open at once in a part (which no -// real score approaches), resolution fails loudly with an exception rather -// than emitting an illegal number. +// real score approaches), resolution refuses the write rather than emitting +// an illegal number. // // == Same-note spans == // @@ -92,10 +93,10 @@ class SpannerResolver SpannerResolver() = default; // Walks inPart in serialization order, assigning a number to every - // identity spanner event and a size to every ottava stop. May be called - // once per part of a score; results accumulate (object addresses are - // unique across parts). - void resolvePart(const api::PartData &inPart); + // identity spanner event and a size to every ottava stop. Reports an + // unmatched ottava stop through diagnostics. May be called once per part + // of a score; results accumulate (object addresses are unique across parts). + void resolvePart(const api::PartData &inPart, int partIndex, DiagnosticsContext diagnostics = {}); // The number the writer should emit for the given spanner object, or // nullopt to omit the attribute. inObject must be the address of the same diff --git a/src/private/mxtest/api/DiagnosticsTest.cpp b/src/private/mxtest/api/DiagnosticsTest.cpp new file mode 100644 index 000000000..fa7ff8ed6 --- /dev/null +++ b/src/private/mxtest/api/DiagnosticsTest.cpp @@ -0,0 +1,242 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mxtest/control/CompileControl.h" +#ifdef MX_COMPILE_API_TESTS + +#include "cpul/cpulTestHarness.h" +#include "mx/api/Diagnostics.h" +#include "mx/api/DirectionData.h" +#include "mx/api/MusicXml.h" +#include "mx/api/OttavaData.h" +#include "mx/api/ScoreData.h" + +#include +#include +#include +#include + +using namespace mx::api; + +inline ScoreData diagnosticsScore(int measureCount) +{ + ScoreData score; + score.ticksPerQuarter = 10; + auto &part = score.parts.emplace_back(); + part.uniqueId = "P1"; + for (int i = 0; i < measureCount; ++i) + { + auto &measure = part.measures.emplace_back(); + auto &staff = measure.staves.emplace_back(); + auto ¬e = staff.voices[0].notes.emplace_back(); + note.durationData.durationTimeTicks = 10; + note.durationData.durationName = DurationName::quarter; + } + return score; +} + +inline void diagnosticsAddOttavaStop(ScoreData &score, int measureIndex, int tick) +{ + DirectionData direction; + direction.tickTimePosition = tick; + direction.directionTypes.emplace_back(DirectionChoice{OttavaStop{}}); + score.parts.front() + .measures.at(static_cast(measureIndex)) + .staves.front() + .directions.emplace_back(direction); +} + +inline std::string diagnosticsTimeSignatureXml() +{ + return R"( + + Music + + + + 1 + + 1 + + 11quarter + + +)"; +} + +TEST(diagnosticsCollectAndInvokeHandlerInOrder, Diagnostics) +{ + auto score = diagnosticsScore(2); + diagnosticsAddOttavaStop(score, 0, 2); + diagnosticsAddOttavaStop(score, 1, 7); + + std::vector callbackMeasures; + Diagnostics diagnostics{[&callbackMeasures](const Diagnostic &diagnostic) { + callbackMeasures.push_back(diagnostic.location.measureIndex); + }}; + const auto result = fromScore(score, diagnostics); + + REQUIRE(result.ok()); + REQUIRE(diagnostics.all().size() == 2); + CHECK_EQUAL(0, diagnostics.all()[0].location.measureIndex); + CHECK_EQUAL(1, diagnostics.all()[1].location.measureIndex); + REQUIRE(callbackMeasures.size() == 2); + CHECK_EQUAL(0, callbackMeasures[0]); + CHECK_EQUAL(1, callbackMeasures[1]); + CHECK(diagnostics.hasSeverityOrWorse(Severity::warning)); + CHECK(!diagnostics.hasSeverityOrWorse(Severity::error)); +} + +T_END + +TEST(unmatchedOttavaDiagnosticHasScoreLocationAndKeepsOutput, Diagnostics) +{ + auto score = diagnosticsScore(2); + diagnosticsAddOttavaStop(score, 1, 7); + + const auto silent = fromScore(score); + Diagnostics diagnostics; + const auto observed = fromScore(score, diagnostics); + REQUIRE(silent.ok()); + REQUIRE(observed.ok()); + REQUIRE(diagnostics.all().size() == 1); + + const auto &diagnostic = diagnostics.all().front(); + CHECK(Severity::warning == diagnostic.severity); + CHECK(DiagnosticCode::unmatchedSpanner == diagnostic.code); + CHECK_EQUAL(0, diagnostic.location.partIndex); + CHECK_EQUAL(1, diagnostic.location.measureIndex); + CHECK_EQUAL(0, diagnostic.location.staffIndex); + CHECK_EQUAL(-1, diagnostic.location.voiceIndex); + CHECK_EQUAL(7, diagnostic.location.tickTimePosition); + CHECK_EQUAL(std::string{"mx: warning at part=0 measure=1 staff=0 tick=7: " + "octave-shift stop has no matching start; using size 8"}, + formatDiagnostic(diagnostic)); + + std::ostringstream silentXml; + std::ostringstream observedXml; + REQUIRE(silent.value().writeToStream(silentXml).ok()); + REQUIRE(observed.value().writeToStream(observedXml).ok()); + CHECK_EQUAL(silentXml.str(), observedXml.str()); +} + +T_END + +TEST(outOfRangeTimeSignatureStaffReportsAdjustment, Diagnostics) +{ + std::istringstream stream{diagnosticsTimeSignatureXml()}; + auto document = MusicXml::fromStream(stream); + REQUIRE(document.ok()); + + Diagnostics diagnostics; + const auto score = getScore(document.value(), diagnostics); + REQUIRE(score.ok()); + REQUIRE(diagnostics.all().size() == 1); + const auto &diagnostic = diagnostics.all().front(); + CHECK(DiagnosticCode::valueAdjusted == diagnostic.code); + CHECK_EQUAL(0, diagnostic.location.partIndex); + CHECK_EQUAL(0, diagnostic.location.measureIndex); + CHECK_EQUAL(-1, diagnostic.location.staffIndex); + CHECK(diagnostic.message.find("staff number 3") != std::string::npos); + CHECK(score.value().parts.front().measures.front().staffTimeSignatures.empty()); + CHECK(!score.value().parts.front().measures.front().timeSignature.isImplicit); + + std::istringstream silentStream{diagnosticsTimeSignatureXml()}; + auto silentDocument = MusicXml::fromStream(silentStream); + REQUIRE(silentDocument.ok()); + const auto silentScore = getScore(silentDocument.value()); + REQUIRE(silentScore.ok()); + CHECK(silentScore.value() == score.value()); +} + +T_END + +TEST(intoScoreCollectsDiagnostics, Diagnostics) +{ + std::istringstream stream{diagnosticsTimeSignatureXml()}; + auto document = MusicXml::fromStream(stream); + REQUIRE(document.ok()); + + Diagnostics diagnostics; + const auto score = intoScore(std::move(document).value(), diagnostics); + REQUIRE(score.ok()); + REQUIRE(diagnostics.all().size() == 1); + CHECK(DiagnosticCode::valueAdjusted == diagnostics.all().front().code); +} + +T_END + +TEST(diagnosticsMaySpanSequentialCalls, Diagnostics) +{ + auto score = diagnosticsScore(1); + diagnosticsAddOttavaStop(score, 0, 0); + Diagnostics diagnostics; + REQUIRE(fromScore(score, diagnostics).ok()); + REQUIRE(fromScore(score, diagnostics).ok()); + CHECK_EQUAL(static_cast(2), diagnostics.all().size()); +} + +T_END + +TEST(throwingDiagnosticHandlerBecomesInternalError, Diagnostics) +{ + auto score = diagnosticsScore(1); + diagnosticsAddOttavaStop(score, 0, 0); + Diagnostics diagnostics{[](const Diagnostic &) { throw std::runtime_error{"handler failed"}; }}; + + const auto result = fromScore(score, diagnostics); + REQUIRE(!result.ok()); + CHECK(ResultCode::internalError == result.error().code); + REQUIRE(result.error().cause); + bool caughtExpected = false; + try + { + std::rethrow_exception(result.error().cause); + } + catch (const std::runtime_error &error) + { + caughtExpected = true; + CHECK_EQUAL(std::string{"handler failed"}, std::string{error.what()}); + } + CHECK(caughtExpected); +} + +T_END + +TEST(tooManyConcurrentSpannersIsLocatedRefusal, Diagnostics) +{ + auto score = diagnosticsScore(2); + auto &startDirections = score.parts.front().measures.front().staves.front().directions; + auto &stopDirections = score.parts.front().measures.back().staves.front().directions; + for (int i = 0; i < 17; ++i) + { + const auto identity = std::string{"ottava-"} + std::to_string(i); + OttavaStart start; + start.spannerStart.number = SpannerNumber{identity}; + DirectionData startDirection; + startDirection.tickTimePosition = 3; + startDirection.directionTypes.emplace_back(DirectionChoice{start}); + startDirections.emplace_back(std::move(startDirection)); + + OttavaStop stop; + stop.spannerStop.number = SpannerNumber{identity}; + DirectionData stopDirection; + stopDirection.tickTimePosition = 6; + stopDirection.directionTypes.emplace_back(DirectionChoice{stop}); + stopDirections.emplace_back(std::move(stopDirection)); + } + + const auto result = fromScore(score); + REQUIRE(!result.ok()); + CHECK(ResultCode::tooManyElements == result.error().code); + CHECK_EQUAL(0, result.error().location.partIndex); + CHECK_EQUAL(0, result.error().location.measureIndex); + CHECK_EQUAL(0, result.error().location.staffIndex); + CHECK_EQUAL(3, result.error().location.tickTimePosition); + CHECK(result.error().message.find("more than 16 spanners") != std::string::npos); +} + +T_END + +#endif From 2e7b6195150afaf8b7ec2857da6c42f1f6885c71 Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Wed, 16 Sep 2026 14:59:21 +0200 Subject: [PATCH 2/2] docs: clarify diagnostics usage and internals --- .agents/skills/mx-api-doctrine/SKILL.md | 2 +- src/include/mx/api/Diagnostics.h | 20 +++++++++++------ src/include/mx/api/MusicXml.h | 27 +++++++++++++++++++---- src/private/mx/api/Diagnostics.cpp | 8 +++---- src/private/mx/api/LocationFormatting.cpp | 12 +++++----- src/private/mx/api/LocationFormatting.h | 2 +- src/private/mx/api/Result.cpp | 8 +++---- src/private/mx/impl/DiagnosticsContext.h | 9 +++++--- src/private/mx/impl/MeasureReader.cpp | 10 ++++----- src/private/mx/impl/MeasureReader.h | 5 +++++ src/private/mx/impl/PartReader.cpp | 25 ++++++++++----------- src/private/mx/impl/PartReader.h | 6 +++++ src/private/mx/impl/ScoreReader.cpp | 6 +++-- src/private/mx/impl/SpannerResolver.cpp | 8 +++---- src/private/mx/utility/Throw.h | 4 ++++ 15 files changed, 99 insertions(+), 53 deletions(-) diff --git a/.agents/skills/mx-api-doctrine/SKILL.md b/.agents/skills/mx-api-doctrine/SKILL.md index b99339bac..4cd73b962 100644 --- a/.agents/skills/mx-api-doctrine/SKILL.md +++ b/.agents/skills/mx-api-doctrine/SKILL.md @@ -43,7 +43,7 @@ Responses to wrong api usage, in order of preference: returns a default-constructed copy; the writer drops the half of an encoding that is meaningless for the note it is on (a tie on a silent cue note is written as `` notation only, never as a sound-level ``). Report an important adjustment through an - optional `Diagnostics` collector; ignoring diagnostics must leave a safe, usable value. + optional `Diagnostics` collector. 3. `Result` (`Result.h`): the error channel of last resort. It exists for the `MusicXml` I/O boundary, where failure is real (unreadable file, unparseable XML). Do not spread it into the data model. diff --git a/src/include/mx/api/Diagnostics.h b/src/include/mx/api/Diagnostics.h index 368e048a4..a10063fdb 100644 --- a/src/include/mx/api/Diagnostics.h +++ b/src/include/mx/api/Diagnostics.h @@ -26,19 +26,20 @@ enum class Severity // The recoverable decision reported by a diagnostic. enum class DiagnosticCode { - valueAdjusted, - unmatchedSpanner + valueAdjusted, // a value was changed to one that MusicXML can represent + unmatchedSpanner // a spanner endpoint had no matching endpoint }; -// A non-fatal problem noticed while producing a value. +// A non-fatal problem noticed while producing a score or MusicXML document. struct Diagnostic { - Severity severity = Severity::warning; - DiagnosticCode code = DiagnosticCode::valueAdjusted; - Location location; - std::string message; + Severity severity = Severity::warning; // effect on the translated value + DiagnosticCode code = DiagnosticCode::valueAdjusted; // kind of recovery performed + Location location; // source or output score position, when known + std::string message; // human-readable description }; +// Receives a diagnostic while a score or MusicXML document is translated. using DiagnosticHandler = std::function; // Collects diagnostics from one or more translations. A handler, when supplied, @@ -49,8 +50,13 @@ class Diagnostics Diagnostics() = default; explicit Diagnostics(DiagnosticHandler handler); + // Adds a diagnostic to this collector and calls its handler. void add(Diagnostic diagnostic); + + // All diagnostics collected so far, in the order they were reported. std::span all() const noexcept; + + // True when the collection contains this severity or a more severe one. bool hasSeverityOrWorse(Severity threshold) const noexcept; private: diff --git a/src/include/mx/api/MusicXml.h b/src/include/mx/api/MusicXml.h index 45c088f82..83959582f 100644 --- a/src/include/mx/api/MusicXml.h +++ b/src/include/mx/api/MusicXml.h @@ -72,9 +72,25 @@ class MusicXml }; // Reads the score out of the document. The document stays alive and can be -// read again or written out. Pass Diagnostics to observe adjustments made -// while translating the document. +// read again or written out. Result getScore(const MusicXml &document); + +// Reads the score and reports any adjustments made while translating it. +// +// To inspect diagnostics after the translation: +// +// Diagnostics diagnostics; +// auto score = getScore(document, diagnostics); +// for (const auto &diagnostic : diagnostics.all()) { +// std::cerr << formatDiagnostic(diagnostic) << '\n'; +// } +// +// To handle diagnostics as they are found: +// +// Diagnostics diagnostics{[](const Diagnostic &diagnostic) { +// std::cerr << formatDiagnostic(diagnostic) << '\n'; +// }}; +// auto score = getScore(document, diagnostics); Result getScore(const MusicXml &document, Diagnostics &diagnostics); // Reads the score out of the document and consumes it: the underlying tree @@ -82,13 +98,16 @@ Result getScore(const MusicXml &document, Diagnostics &diagnostics); // goes out of scope. Pass the document with std::move, or hand over the // Result's value directly. Result intoScore(MusicXml document); + +// Consumes the document and reports any adjustments made while reading it. Result intoScore(MusicXml document, Diagnostics &diagnostics); // Authors a new document from ScoreData. Fails with an error result when the // ScoreData describes something the core model will not represent (e.g. more -// than 8 beams) rather than silently dropping data. Pass Diagnostics to -// observe recoverable authoring decisions. +// than 8 beams) rather than silently dropping data. Result fromScore(const ScoreData &score); + +// Authors a document and reports recoverable authoring decisions. Result fromScore(const ScoreData &score, Diagnostics &diagnostics); } // namespace api diff --git a/src/private/mx/api/Diagnostics.cpp b/src/private/mx/api/Diagnostics.cpp index 0ae696389..c6e95dfdf 100644 --- a/src/private/mx/api/Diagnostics.cpp +++ b/src/private/mx/api/Diagnostics.cpp @@ -58,10 +58,10 @@ std::string formatDiagnostic(const Diagnostic &diagnostic) break; } - std::string text{"mx: "}; - text += severityName; - appendFormattedLocationAndMessage(text, diagnostic.location, diagnostic.message); - return text; + std::string result{"mx: "}; + result += severityName; + result += formatLocationAndMessage(diagnostic.location, diagnostic.message); + return result; } } // namespace api } // namespace mx diff --git a/src/private/mx/api/LocationFormatting.cpp b/src/private/mx/api/LocationFormatting.cpp index 9ec425307..f68ad57c8 100644 --- a/src/private/mx/api/LocationFormatting.cpp +++ b/src/private/mx/api/LocationFormatting.cpp @@ -8,8 +8,9 @@ namespace mx { namespace api { -void appendFormattedLocationAndMessage(std::string &text, const Location &location, const std::string &message) +std::string formatLocationAndMessage(const Location &location, const std::string &message) { + std::string result; std::string where; const auto appendWhere = [&where](const char *name, long long value) { if (value < 0) @@ -35,15 +36,16 @@ void appendFormattedLocationAndMessage(std::string &text, const Location &locati if (!where.empty()) { - text += " at "; - text += where; + result += " at "; + result += where; } if (!message.empty()) { - text += ": "; - text += message; + result += ": "; + result += message; } + return result; } } // namespace api } // namespace mx diff --git a/src/private/mx/api/LocationFormatting.h b/src/private/mx/api/LocationFormatting.h index b130f4ba3..42510f031 100644 --- a/src/private/mx/api/LocationFormatting.h +++ b/src/private/mx/api/LocationFormatting.h @@ -12,6 +12,6 @@ namespace mx { namespace api { -void appendFormattedLocationAndMessage(std::string &text, const Location &location, const std::string &message); +std::string formatLocationAndMessage(const Location &location, const std::string &message); } // namespace api } // namespace mx diff --git a/src/private/mx/api/Result.cpp b/src/private/mx/api/Result.cpp index d1af5d9a6..1d7b3fd89 100644 --- a/src/private/mx/api/Result.cpp +++ b/src/private/mx/api/Result.cpp @@ -53,10 +53,10 @@ std::string formatError(const ApiError &error) break; } - std::string text{"mx: "}; - text += codeName; - appendFormattedLocationAndMessage(text, error.location, error.message); - return text; + std::string result{"mx: "}; + result += codeName; + result += formatLocationAndMessage(error.location, error.message); + return result; } } // namespace api } // namespace mx diff --git a/src/private/mx/impl/DiagnosticsContext.h b/src/private/mx/impl/DiagnosticsContext.h index 918c904b1..c0ce5662c 100644 --- a/src/private/mx/impl/DiagnosticsContext.h +++ b/src/private/mx/impl/DiagnosticsContext.h @@ -7,6 +7,8 @@ #include "mx/api/Diagnostics.h" #include "mx/impl/MeasureCursor.h" +#include +#include #include #include @@ -19,7 +21,8 @@ class DiagnosticsContext public: DiagnosticsContext() = default; - explicit DiagnosticsContext(api::Diagnostics &diagnostics) : myDiagnostics{&diagnostics} + explicit DiagnosticsContext(api::Diagnostics &diagnostics) + : myDiagnostics{std::make_shared>(diagnostics)} { } @@ -27,7 +30,7 @@ class DiagnosticsContext { if (myDiagnostics) { - myDiagnostics->add(api::Diagnostic{severity, code, std::move(location), std::move(message)}); + myDiagnostics->get().add(api::Diagnostic{severity, code, std::move(location), std::move(message)}); } } @@ -44,7 +47,7 @@ class DiagnosticsContext } private: - api::Diagnostics *myDiagnostics = nullptr; + std::shared_ptr> myDiagnostics; }; } // namespace impl } // namespace mx diff --git a/src/private/mx/impl/MeasureReader.cpp b/src/private/mx/impl/MeasureReader.cpp index 3b4adc0c1..8a26d7b23 100644 --- a/src/private/mx/impl/MeasureReader.cpp +++ b/src/private/mx/impl/MeasureReader.cpp @@ -69,7 +69,7 @@ namespace mx { namespace impl { -api::FigureData measureReaderParseFigure(const core::Figure &figure) +api::FigureData MeasureReader::parseFigure(const core::Figure &figure) { api::FigureData figureData; @@ -91,8 +91,8 @@ api::FigureData measureReaderParseFigure(const core::Figure &figure) return figureData; } -int measureReaderFiguredBassStaffIndex(const MeasureCursor &cursor, const api::MeasureData &measure, - const core::Note *nextNotePtr) +int MeasureReader::figuredBassStaffIndex(const MeasureCursor &cursor, const api::MeasureData &measure, + const core::Note *nextNotePtr) { auto staffIndex = cursor.staffIndex; @@ -828,7 +828,7 @@ void MeasureReader::parseFiguredBass(const core::FiguredBass &inMxFiguredBass, c for (const auto &figure : inMxFiguredBass.figure()) { - figuredBass.figures.emplace_back(measureReaderParseFigure(figure)); + figuredBass.figures.emplace_back(parseFigure(figure)); } if (inMxFiguredBass.parentheses().has_value()) @@ -856,7 +856,7 @@ void MeasureReader::parseFiguredBass(const core::FiguredBass &inMxFiguredBass, c direction.figuredBasses.emplace_back(std::move(figuredBass)); - const auto staffIndex = measureReaderFiguredBassStaffIndex(myCurrentCursor, myOutMeasureData, nextNotePtr); + const auto staffIndex = figuredBassStaffIndex(myCurrentCursor, myOutMeasureData, nextNotePtr); myOutMeasureData.staves.at(static_cast(staffIndex)).directions.emplace_back(std::move(direction)); } diff --git a/src/private/mx/impl/MeasureReader.h b/src/private/mx/impl/MeasureReader.h index 4cadd37ce..e075fec2e 100644 --- a/src/private/mx/impl/MeasureReader.h +++ b/src/private/mx/impl/MeasureReader.h @@ -25,6 +25,7 @@ class Direction; class Attributes; class Harmony; class FiguredBass; +class Figure; class Print; class Sound; class Barline; @@ -80,6 +81,10 @@ class MeasureReader mutable int myPreviousNoteBucketStaffIndex; private: + static api::FigureData parseFigure(const core::Figure &figure); + static int figuredBassStaffIndex(const MeasureCursor &cursor, const api::MeasureData &measure, + const core::Note *nextNotePtr); + void addStavesToOutMeasure() const; void parseTimeSignature() const; diff --git a/src/private/mx/impl/PartReader.cpp b/src/private/mx/impl/PartReader.cpp index 600b55a03..180cdd9ee 100644 --- a/src/private/mx/impl/PartReader.cpp +++ b/src/private/mx/impl/PartReader.cpp @@ -42,12 +42,12 @@ namespace impl { // True when a / carries any of the formatting // attributes that MusicXML 2.0 deprecated in favor of the *-display elements. -bool partReaderNameHasDeprecatedFormatting(const core::PartName &n) +bool PartReader::nameHasDeprecatedFormatting(const core::PartName &name) { - return n.fontFamily().has_value() || n.fontStyle().has_value() || n.fontSize().has_value() || - n.fontWeight().has_value() || n.color().has_value() || n.defaultX().has_value() || - n.defaultY().has_value() || n.relativeX().has_value() || n.relativeY().has_value() || - n.justify().has_value(); + return name.fontFamily().has_value() || name.fontStyle().has_value() || name.fontSize().has_value() || + name.fontWeight().has_value() || name.color().has_value() || name.defaultX().has_value() || + name.defaultY().has_value() || name.relativeX().has_value() || name.relativeY().has_value() || + name.justify().has_value(); } // Reads a name/abbreviation's display text and formatting into the api's single @@ -55,15 +55,15 @@ bool partReaderNameHasDeprecatedFormatting(const core::PartName &n) // a present *-display element is canonical and wins; otherwise any deprecated // formatting on the name element itself is migrated into the display model so it // is re-emitted at the modern location. -void partReaderReadNameDisplay(const core::PartName &nameElement, const std::optional &display, - std::string &outText, api::PrintData &outPrintData, api::PositionData &outPositionData) +void PartReader::readNameDisplay(const core::PartName &nameElement, const std::optional &display, + std::string &outText, api::PrintData &outPrintData, api::PositionData &outPositionData) { if (display.has_value()) { outText = extractDisplayText(*display); extractDisplayFormatting(*display, outPrintData, outPositionData); } - else if (partReaderNameHasDeprecatedFormatting(nameElement)) + else if (nameHasDeprecatedFormatting(nameElement)) { outText = nameElement.value(); outPrintData = getPrintData(nameElement); @@ -210,17 +210,16 @@ void PartReader::parseScorePart() const const auto &corePartName = myScorePart.partName(); myOutPartData.name = corePartName.value(); myOutPartData.namePrintObject = getPrintObject(corePartName); - partReaderReadNameDisplay(corePartName, myScorePart.partNameDisplay(), myOutPartData.displayName, - myOutPartData.displayNamePrintData, myOutPartData.displayNamePositionData); + readNameDisplay(corePartName, myScorePart.partNameDisplay(), myOutPartData.displayName, + myOutPartData.displayNamePrintData, myOutPartData.displayNamePositionData); if (myScorePart.partAbbreviation().has_value()) { const auto &coreAbbreviation = *myScorePart.partAbbreviation(); myOutPartData.abbreviation = coreAbbreviation.value(); myOutPartData.abbreviationPrintObject = getPrintObject(coreAbbreviation); - partReaderReadNameDisplay(coreAbbreviation, myScorePart.partAbbreviationDisplay(), - myOutPartData.displayAbbreviation, myOutPartData.displayAbbreviationPrintData, - myOutPartData.displayAbbreviationPositionData); + readNameDisplay(coreAbbreviation, myScorePart.partAbbreviationDisplay(), myOutPartData.displayAbbreviation, + myOutPartData.displayAbbreviationPrintData, myOutPartData.displayAbbreviationPositionData); } else if (myScorePart.partAbbreviationDisplay().has_value()) { diff --git a/src/private/mx/impl/PartReader.h b/src/private/mx/impl/PartReader.h index 206f0f3ee..c3a5b7c12 100644 --- a/src/private/mx/impl/PartReader.h +++ b/src/private/mx/impl/PartReader.h @@ -20,6 +20,8 @@ class PartwisePart; class ScorePart; class ScoreInstrument; class VirtualInstrument; +class PartName; +class NameDisplay; class ScorePartMIDIGroup; class MIDIInstrument; } // namespace core @@ -37,6 +39,10 @@ class PartReader impl::MeasureCursor getCursor() const; private: + static bool nameHasDeprecatedFormatting(const core::PartName &name); + static void readNameDisplay(const core::PartName &nameElement, const std::optional &display, + std::string &outText, api::PrintData &outPrintData, api::PositionData &outPositionData); + const core::PartwisePart &myPartwisePart; const core::ScorePart &myScorePart; int myNumStaves; diff --git a/src/private/mx/impl/ScoreReader.cpp b/src/private/mx/impl/ScoreReader.cpp index 59fcedbbf..c855e71db 100644 --- a/src/private/mx/impl/ScoreReader.cpp +++ b/src/private/mx/impl/ScoreReader.cpp @@ -267,8 +267,10 @@ api::ScoreData ScoreReader::getScoreData() const int divisionsValue = -1; for (const auto &reconciledPart : partMap) { - PartReader reader{*reconciledPart.first, *reconciledPart.second, myOutScoreData.ticksPerQuarter, - myScorePartwise, divisionsValue, myDiagnostics}; + const auto &scorePart = *reconciledPart.first; + const auto &partwisePart = *reconciledPart.second; + const auto ticksPerQuarter = myOutScoreData.ticksPerQuarter; + PartReader reader{scorePart, partwisePart, ticksPerQuarter, myScorePartwise, divisionsValue, myDiagnostics}; myOutScoreData.parts.emplace_back(reader.getPartData()); const auto cursorReturn = reader.getCursor(); divisionsValue = cursorReturn.ticksPerQuarter; diff --git a/src/private/mx/impl/SpannerResolver.cpp b/src/private/mx/impl/SpannerResolver.cpp index a6253fc27..5bd711312 100644 --- a/src/private/mx/impl/SpannerResolver.cpp +++ b/src/private/mx/impl/SpannerResolver.cpp @@ -397,10 +397,10 @@ static void spannerNumberAssignClass(const std::vector &inEv } if (chosen == 0) { - throw WriteRefusal{api::ApiError{ - api::ResultCode::tooManyElements, group.location, - "more than 16 spanners of one type are open at the same point; MusicXML number attributes only " - "range from 1 to 16"}}; + MX_THROW_AS(WriteRefusal, + (api::ApiError{api::ResultCode::tooManyElements, group.location, + "more than 16 spanners of one type are open at the same point; MusicXML number " + "attributes only range from 1 to 16"})) } occupied[chosen].push_back(group.interval); for (const void *object : group.objects) diff --git a/src/private/mx/utility/Throw.h b/src/private/mx/utility/Throw.h index 06d14fd2e..3775ca57d 100644 --- a/src/private/mx/utility/Throw.h +++ b/src/private/mx/utility/Throw.h @@ -34,6 +34,10 @@ #define MX_THROW(throw_error_message) throw std::runtime_error(MX_ERROR_MESSAGE(throw_error_message)); #endif +#ifndef MX_THROW_AS +#define MX_THROW_AS(exception_type, ...) throw exception_type(__VA_ARGS__); +#endif + #ifndef MX_LOG #define MX_LOG(message) std::cout << MX_ERROR_MESSAGE(message) << std::endl; #endif