From 8f50fb18bfba13656f03390c67f4877bbab8dc42 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 06:28:49 +0200 Subject: [PATCH 1/2] refactor: make reader invariants and fallbacks explicit --- src/odr/document.cpp | 1 + src/odr/document_element.hpp | 1 - src/odr/internal/cfb/cfb_util.cpp | 6 +----- .../internal/common/sheet_recalculation.cpp | 1 + src/odr/internal/html/filesystem.cpp | 2 +- src/odr/internal/odf/odf_sheet_references.cpp | 6 +++--- src/odr/internal/ooxml/ooxml_util.cpp | 2 +- .../ooxml_spreadsheet_references.cpp | 11 +++++----- .../spreadsheet/ooxml_spreadsheet_style.cpp | 2 +- src/odr/internal/pdf/pdf_page_extractor.cpp | 20 +++++++++---------- src/odr/internal/png/png_util.cpp | 2 +- src/odr/style.hpp | 2 +- 12 files changed, 26 insertions(+), 30 deletions(-) diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 4e425f9d7..37fefcc95 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -626,6 +626,7 @@ void Document::recalculate_edits_() const { try { recalculate(); } catch (const UnsupportedOperation &) { + return; } } diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index 327d74876..c365aee80 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -25,7 +25,6 @@ struct PageLayout; } // namespace odr namespace odr::internal::abstract { -class Document; class ElementAdapter; class TextRootAdapter; diff --git a/src/odr/internal/cfb/cfb_util.cpp b/src/odr/internal/cfb/cfb_util.cpp index 3c49a2c3c..43372b820 100644 --- a/src/odr/internal/cfb/cfb_util.cpp +++ b/src/odr/internal/cfb/cfb_util.cpp @@ -190,11 +190,7 @@ void Archive::Iterator::enter_(Entry entry) { } void Archive::Iterator::dig_left_() { - if (!m_entry.has_value()) { - return; - } - - while (true) { + while (m_entry.has_value()) { const std::optional left = m_entry->left(); if (!left.has_value()) { break; diff --git a/src/odr/internal/common/sheet_recalculation.cpp b/src/odr/internal/common/sheet_recalculation.cpp index 97af3f783..807f5830b 100644 --- a/src/odr/internal/common/sheet_recalculation.cpp +++ b/src/odr/internal/common/sheet_recalculation.cpp @@ -355,6 +355,7 @@ internal::recalculate(const abstract::Document &document) { position.cell.row, CellValue(ValueType::unknown)); } catch (const UnsupportedOperation &) { + continue; } continue; } diff --git a/src/odr/internal/html/filesystem.cpp b/src/odr/internal/html/filesystem.cpp index 7d5cfc59f..f272adc9e 100644 --- a/src/odr/internal/html/filesystem.cpp +++ b/src/odr/internal/html/filesystem.cpp @@ -65,7 +65,7 @@ std::optional entry_data_url(const File &file, /// a path that escapes the output directory or collides with the listing gets /// none, and stays inline instead. std::optional entry_location(const Path &path) { - const RelPath relative = path.make_relative(); + RelPath relative = path.make_relative(); if (relative.escaping() || relative.empty() || relative.string() == listing_path) { return std::nullopt; diff --git a/src/odr/internal/odf/odf_sheet_references.cpp b/src/odr/internal/odf/odf_sheet_references.cpp index 651f4a74d..6262a92b1 100644 --- a/src/odr/internal/odf/odf_sheet_references.cpp +++ b/src/odr/internal/odf/odf_sheet_references.cpp @@ -92,7 +92,7 @@ constexpr std::array condition_attributes{ /// The attributes stating the cell a condition is relative to, whose sheet /// is the one a reference of the condition names by stating none. -constexpr std::array base_cell_attributes{ +constexpr std::array base_cell_attributes{ "table:base-cell-address", "calcext:base-cell-address", "style:base-cell-address"}; @@ -165,8 +165,8 @@ void move_subtree(const pugi::xml_node node, std::string sheet, // Where a delete removes that cell, it reads from the first one that stays. std::string own = sheet; std::int64_t shift = 0; - for (const std::string_view base : base_cell_attributes) { - pugi::xml_attribute address = node.attribute(base.data()); + for (const char *const base : base_cell_attributes) { + pugi::xml_attribute address = node.attribute(base); std::optional cell = address ? formula::parse("[" + std::string(address.value()) + "]", formula::Syntax::opendocument) diff --git a/src/odr/internal/ooxml/ooxml_util.cpp b/src/odr/internal/ooxml/ooxml_util.cpp index 722aee567..3622c681c 100644 --- a/src/odr/internal/ooxml/ooxml_util.cpp +++ b/src/odr/internal/ooxml/ooxml_util.cpp @@ -582,7 +582,7 @@ std::vector ooxml::parse_relationship_targets( result.push_back( resolve_part_path(path, e.node().attribute("Target").value())); } catch (const std::invalid_argument &) { - // An invalid optional relationship contributes no part. + continue; // Ignore an invalid optional relationship. } } return result; diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_references.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_references.cpp index 51d9c1acb..b566eb418 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_references.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_references.cpp @@ -245,9 +245,10 @@ void move_shared_group(const std::vector &members, shifted(*expression, master->position, member.position); formula::move_references(expected, edit, sheet); std::string text = formula::to_string(expected, syntax); - kept = kept && - formula::to_string(shifted(moved_master, *master_at, *member_at), - syntax) == text; + if (kept && master_at.has_value()) { + kept = formula::to_string(shifted(moved_master, *master_at, *member_at), + syntax) == text; + } written.emplace_back(member.formula, std::move(text)); } @@ -324,7 +325,7 @@ void move_calc_chain(pugi::xml_node calc_chain, /// An element stating a range list, which goes when a delete takes the list. struct RangeAttribute final { std::string_view element; - std::string_view attribute; + const char *attribute; }; constexpr std::array removable_ranges{{ @@ -427,7 +428,7 @@ void collect_ranges(const pugi::xml_node node, const formula::SheetEdit &edit, } bool removed = false; for (const RangeAttribute &range : removable_ranges) { - pugi::xml_attribute attribute = child.attribute(range.attribute.data()); + pugi::xml_attribute attribute = child.attribute(range.attribute); if (name != range.element || !attribute) { continue; } diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp index 59ac47300..6a54f474b 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp @@ -481,7 +481,7 @@ void StyleRegistry::generate_indices_(const pugi::xml_node styles_root) { format.attribute("numFmtId").as_uint(), number_format::Format(format.attribute("formatCode").value())); } catch (const std::invalid_argument &) { - // a code this cannot read shows its value as `General` does + continue; // Fall back to General for an unsupported code. } } } diff --git a/src/odr/internal/pdf/pdf_page_extractor.cpp b/src/odr/internal/pdf/pdf_page_extractor.cpp index 7ba46de38..79a531389 100644 --- a/src/odr/internal/pdf/pdf_page_extractor.cpp +++ b/src/odr/internal/pdf/pdf_page_extractor.cpp @@ -842,7 +842,7 @@ void show_type3(std::vector &out, const Resources &resources, GraphicsState &state, const Logger &logger, std::set &warned, ActiveForms &active, MarkedContentStack &marked, std::optional &pen, - const std::string &codes, Font *font) { + const std::string &codes, Font *font, const Type3Data &type3) { // `show` advances the text matrix by the whole segment; snapshot `Tm` so the // per-glyph loop below can replay the advance to place each char proc. const util::math::Transform2D saved_matrix = state.current().text.matrix; @@ -857,7 +857,7 @@ void show_type3(std::vector &out, const Resources &resources, return; } - if (font->type3->char_procs.empty() || state.type3_depth > 8) { + if (type3.char_procs.empty() || state.type3_depth > 8) { return; // nothing to draw, or pathological Type3 recursion } ++state.type3_depth; @@ -874,10 +874,9 @@ void show_type3(std::vector &out, const Resources &resources, font->encoding ? font->encoding->glyph_name(static_cast(code)) : std::string_view{}; - const auto it = name.empty() - ? font->type3->char_procs.end() - : font->type3->char_procs.find(std::string(name)); - if (it != font->type3->char_procs.end()) { + const auto it = name.empty() ? type3.char_procs.end() + : type3.char_procs.find(std::string(name)); + if (it != type3.char_procs.end()) { const auto ¤t = state.current(); const GraphicsState::Text &text = current.text; // glyph space -> user space = /FontMatrix x [size params] x Tm x CTM. @@ -886,11 +885,10 @@ void show_type3(std::vector &out, const Resources &resources, text.size * text.horizontal_scaling / 100.0, text.size, 0, text.rise); const util::math::Transform2D glyph_to_user = - font->type3->font_matrix * size_params * text.matrix * + type3.font_matrix * size_params * text.matrix * current.general.transform_matrix; - const Resources &scope = font->type3->resources != nullptr - ? *font->type3->resources - : resources; + const Resources &scope = + type3.resources != nullptr ? *type3.resources : resources; // Scoped like `q`/`Q`, but pinned: the char proc's own `q`/`Q` cannot // escape it and disturb the text state driving the glyph loop. @@ -919,7 +917,7 @@ void run_content(const std::string &content, const Resources &resources, const auto show_run = [&](const std::string &codes, Font *font) { if (font != nullptr && font->type3) { show_type3(out, resources, state, logger, warned, active, marked, pen, - codes, font); + codes, font, *font->type3); } else { show(out, state, marked, pen, codes, font); } diff --git a/src/odr/internal/png/png_util.cpp b/src/odr/internal/png/png_util.cpp index 7bbc92c11..a3cf6abd7 100644 --- a/src/odr/internal/png/png_util.cpp +++ b/src/odr/internal/png/png_util.cpp @@ -60,7 +60,7 @@ std::string assemble(const std::string &rows, const std::size_t stride, const std::string compressed = crypto::util::zlib_deflate(raw); std::string_view remaining = compressed; // PNG 5.3 limits chunk lengths to 2^31-1; small chunks avoid narrowing. - constexpr std::size_t chunk_size = 64 * 1024; + constexpr std::size_t chunk_size = std::size_t{64} * 1024; while (!remaining.empty()) { const std::size_t count = std::min(remaining.size(), chunk_size); write_chunk(out, "IDAT", remaining.substr(0, count)); diff --git a/src/odr/style.hpp b/src/odr/style.hpp index b84b162fb..367e2382e 100644 --- a/src/odr/style.hpp +++ b/src/odr/style.hpp @@ -125,7 +125,7 @@ template struct DirectionalStyle final { std::optional bottom; DirectionalStyle() = default; - explicit DirectionalStyle(std::optional all) + explicit DirectionalStyle(const std::optional &all) : right{all}, top{all}, left{all}, bottom{all} {} void override(const DirectionalStyle &other) { From c60c2155bb63da3afa81883dc8783749814f728b Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 6 Oct 2026 15:19:54 +0200 Subject: [PATCH 2/2] refactor: keep the reasons beside the explicit fallbacks The comments at the OOXML relationship and number-format fallbacks said why the code continues. The rewrite only repeated the continue, so the original reasons stand above it again. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MxyTMutqSUJRGfxA8CyzMc --- src/odr/internal/ooxml/ooxml_util.cpp | 3 ++- src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/odr/internal/ooxml/ooxml_util.cpp b/src/odr/internal/ooxml/ooxml_util.cpp index 3622c681c..d99ade21e 100644 --- a/src/odr/internal/ooxml/ooxml_util.cpp +++ b/src/odr/internal/ooxml/ooxml_util.cpp @@ -582,7 +582,8 @@ std::vector ooxml::parse_relationship_targets( result.push_back( resolve_part_path(path, e.node().attribute("Target").value())); } catch (const std::invalid_argument &) { - continue; // Ignore an invalid optional relationship. + // An invalid optional relationship contributes no part. + continue; } } return result; diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp index 6a54f474b..6f90e4491 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp @@ -481,7 +481,8 @@ void StyleRegistry::generate_indices_(const pugi::xml_node styles_root) { format.attribute("numFmtId").as_uint(), number_format::Format(format.attribute("formatCode").value())); } catch (const std::invalid_argument &) { - continue; // Fall back to General for an unsupported code. + // a code this cannot read shows its value as `General` does + continue; } } }