Skip to content

Collapse the duplicated enum tables and XML helpers the persistence layer maintains twice (ponytail audit) #563

Description

@JArmandoAnaya

A repo-wide over-engineering audit (244k tracked source lines, all 13
FetchContent deps, 9 CMake options, 14 CI jobs) found the tree broadly
disciplined: no dead public API (all 262 RM_API functions have a
non-test caller), no unused dependency, no dead build flag, one
orphaned tracked image.

What it did find is a cluster of deliberate duplication, most of it in the
persistence layer: the OpenDRIVE and OpenSCENARIO reader/writer pairs each keep
their own copy of the same XML helpers and their own one-directional
enum↔string table — several with comments explicitly defending the copy
against a divergence that has not happened in the project's lifetime.

That duplication is not cosmetic. #476 was exactly this class of defect
the writer rewrote lane/mark enums into different semantics than the reader
parsed, because the two tables were maintained separately. The probe on that
issue found 9 corruptions where the issue body listed 3. Collapsing each pair
to one table makes the class unrepresentable rather than merely fixed.

Scope — two sequential PRs, no behavior change

PR-A — kernel dedup (≈ -235 lines)

what where
A1 One constexpr table per enum, serving both directions, replacing 6 reader if-chains + 6 writer switches new core/src/xodr/enum_names.hpp
A2 One copy of to_double / num / set_num / set_optional_num / append_fragment / node_to_string / trim new core/src/xml/xml_common.hpp
A3 linked_end is byte-identical in two files core/src/road/junction_adjacency.hpp
A4 is_one_of hand-rolls a loop the stdlib ships std::ranges::find
A5 Environment::procedural_sky is set, asserted by two tests, and never read by GLRenderer delete

Three invariants A1 must not violate, each preserving today's behaviour:

  1. Write-fallbacks are not table rows. LaneType::Other → "none",
    ObjectType::Other → "none", RoadMarkType::Other → "solid" are the
    writer's fallback argument. As table rows they would make the reader map
    "solid"RoadMarkType::Other — the fastest way to reintroduce xodr: unknown enum spellings are rewritten on save — lane type → "none", road-mark type → "solid", colour → "standard" #476.
  2. Empty-string policies stay at the call site. value_of returns
    nullopt for unknown; the reader keeps its own Other/nullopt policy.
  3. type_str preservation is untouched — the writer already prefers the
    verbatim spelling over the enum name.

PR-B — docs-site (≈ -175 lines)

  • B1 Delete docs-site/scripts/report-external-links.mjs — 125 lines of
    hand-rolled fetch + concurrency + timeout for a report that always exits 0.
    lychee is already a CI dependency; a weekly non-blocking lychee job without
    --offline replaces it in ~15 lines of YAML.
  • B2 Delete docs-site/scripts/theme-css.mjs — a JavaScript parser of C++
    source (indexOf + a QColor(0x..) regex over theme.cpp) that breaks on
    any reformat. The repo already solved this for editor/resources/help/help.css:
    generate in C++, commit the output, byte-gate it from a gtest. Reuse that.

Acceptance — byte-identical output, not green tests

Tests can pass while a round trip silently changes bytes; that is how #476
survived. A throwaway differential probe reads and rewrites all 126 tracked
.xodr files plus tests/esmini/*.xosc on main and on the branch, and
diffs the bytes. Zero differences is the gate, and the gate itself is
sabotage-checked (swap two spellings in the table; the probe must go red).

Declined by the maintainer, deliberately out of scope

  • Cutting the Renderer interface (10 pure virtuals, one implementation,
    no test double). It is a documented architecture non-negotiable in the
    project guide; kept.
  • Recompressing the 13 MB of docs/design/ui-revamp/ PNGs. Git history
    already carries the original bytes, so recompressing grows the repo net
    unless history is rewritten. Not worth it for design screenshots.
  • point_segment_distance (identical in geometry/simplify.cpp and
    mesh/junction_corner_detail.cpp) — the two subsystems share no internal
    header, and creating one to save 13 lines is break-even.
  • The PhaseSemantics read/write twin — one enum, six values, ~30 lines.
    Not worth a new header.

Net: ≈ -410 lines, 0 dependencies removed (every one of the 13 is
load-bearing; sharp is already stubbed for licence reasons).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactorComplexity/duplication reduction — no behavior change

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions