diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 214d319..57d84e8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,7 +9,8 @@ on: jobs: build-and-test: - # Two compilers times two build modes, so four jobs. + # Two platforms, two compilers and two build modes, minus a combination + # that would be a duplicate, so six jobs. # # Both compilers, so a warning or a standard-library difference that only # one of them notices still fails the build. @@ -18,11 +19,26 @@ jobs: # the optimised build is what ships, and the sanitised one is the only # thing that reports a read past the end of a buffer rather than returning # whatever was next to it. + # + # Both platforms, because everything the app does outside the standard + # library is POSIX rather than Linux -- termios, ioctl, dirent, wcwidth -- + # and "should be portable" is a claim nobody had checked. The macOS runner + # is Apple Silicon, so it also builds for arm64, where `char` is unsigned + # by default and x86 says it is signed. That is a real class of bug this + # cannot otherwise see. strategy: fail-fast: false matrix: + os: [ubuntu-latest, macos-latest] cxx: [g++, clang++] mode: [optimised, sanitised] + exclude: + # `g++` on a macOS runner is a shim for Apple clang rather than GCC, + # so this pair would run the clang entry again under another name. + # Real GCC is a `brew install` away; what the macOS runner is here to + # catch is the platform, not a third compiler. + - os: macos-latest + cxx: g++ include: # -Werror lives here rather than in the Makefile on purpose: a # warning should stop a change being merged, not stop a contributor @@ -40,7 +56,7 @@ jobs: -fsanitize=address,undefined -fno-sanitize-recover=undefined -fno-omit-frame-pointer - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} # Set for the job rather than passed to each make: the Makefile takes both # with ?=, and the test and golden targets have to be built with the same diff --git a/CHANGELOG.md b/CHANGELOG.md index 0de8943..5be6ef6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,40 @@ Notable changes per release. Dates are the release date; the PR numbers link the detail, which is where the reasoning lives. +## Unreleased + +### Fixed + +- **`FLASHTERM_SEED` now fixes the review order on every platform, not just the + one you built on.** The shuffle went through `std::shuffle`, whose output the + standard does not specify — only that the permutation is uniformly random — + so libstdc++ and libc++ deal the same seeded deck in different orders. It is + now an explicit Fisher–Yates over `std::mt19937`, whose own output *is* + specified exactly, so the seed alone decides the order. + + Found by the macOS runner below, on its first run, which is the entire + argument for having added it: every multi-card golden case failed there, + because a scripted session answers cards in the order it expects to meet + them and the transcripts were recorded against libstdc++. Nothing was wrong + with the shuffle's randomness, and no real session is affected — the bug was + in what the seed promised. + +### Changed + +- **CI builds and tests on macOS as well as Linux.** Everything FlashTerm does + outside the standard library is POSIX rather than Linux — `termios`, `ioctl`, + `dirent`, `wcwidth` — and the time functions have had their `gmtime_r` and + `localtime_r` branches since the log landed, so this was expected to pass. + Expected is not tested, and "runs on a Mac" is the kind of claim a README + should not make on the strength of reading the source. + + The macOS runner is Apple Silicon, so it also builds for arm64. That is the + half worth having beyond the platform itself: `char` is unsigned there and + signed on x86, which is a real difference that no amount of running on one + architecture can show. `g++` on macOS is a shim for Apple clang rather than + GCC, so that pair is excluded rather than run as a second clang under another + name — six jobs, not eight. + ## 0.3.0 — 2026-08-19 Pictures on cards, sync that puts two machines' reviews back together, and a CI diff --git a/README.md b/README.md index 974f10d..206ae55 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,10 @@ FlashTerm is a terminal flashcard app that makes you type the answer. No multipl ## Quick Start -Needs a C++17 compiler (`g++` or `clang++`) and `make`. +Needs a C++17 compiler (`g++` or `clang++`) and `make`. Linux and macOS are +both built and tested on every change; anything else POSIX will very likely +work, since nothing outside the standard library is used that POSIX does not +define. ```bash git clone https://github.com/Aduneer/FlashTerm @@ -633,7 +636,7 @@ cannot drive an app that insists on a tty. ## Contributing -Issues and pull requests are welcome. `make check` should pass before you open one; CI runs both suites on gcc and clang, in an optimised build and again under `-fsanitize=address,undefined`, all four with `-Werror`. Warnings are deliberately not errors in the Makefile itself, so a warning never stops you building — it stops the change being merged. +Issues and pull requests are welcome. `make check` should pass before you open one; CI runs both suites on Linux and macOS, on gcc and clang, in an optimised build and again under `-fsanitize=address,undefined`, all of them with `-Werror`. Warnings are deliberately not errors in the Makefile itself, so a warning never stops you building — it stops the change being merged. Released versions and what changed in them are in [CHANGELOG.md](CHANGELOG.md). diff --git a/src/review.cpp b/src/review.cpp index c83c0c7..c13dacb 100644 --- a/src/review.cpp +++ b/src/review.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -89,6 +90,51 @@ std::mt19937& rng() { return generator; } +// An index in [0, bound), drawn by hand rather than with +// std::uniform_int_distribution, whose mapping from generator output to result +// is not specified and does differ between standard libraries. See +// deterministic_shuffle. +// +// Rejection sampling: mt19937 covers the whole 32-bit range, so draws from the +// last, short block above the largest exact multiple of `bound` are discarded +// rather than folded in, which would make the low values fractionally likelier. +std::uint32_t bounded(std::uint32_t bound) { + const std::uint64_t range = std::uint64_t{1} << 32; + const std::uint64_t limit = range - (range % bound); + std::uint64_t draw; + do { + draw = rng()(); + } while (draw >= limit); + return static_cast(draw % bound); +} + +// Fisher-Yates, written out rather than calling std::shuffle. +// +// std::shuffle's output is not specified by the standard -- only that the +// result is a uniformly random permutation -- so libstdc++ and libc++ produce +// *different orders from the same seed*. FLASHTERM_SEED is documented as +// fixing the review order, and a promise that holds only on the standard +// library you happened to build against is not the promise it makes. The +// golden suite is what noticed: every multi-card case failed on macOS, because +// a scripted session answers cards in the order it expects to meet them, and +// the transcripts were recorded against libstdc++. +// +// mt19937 itself is safe to keep -- the standard specifies its algorithm +// exactly, down to a required value for the 10000th draw -- so pinning the +// permutation is only a matter of not letting the library choose how the +// numbers become indices. +template +void deterministic_shuffle(Iterator first, Iterator last) { + const std::size_t count = static_cast(last - first); + for (std::size_t i = count; i > 1; --i) { + const std::size_t pick = bounded(static_cast(i)); + if (pick != i - 1) { + std::iter_swap(first + static_cast(i - 1), + first + static_cast(pick)); + } + } +} + int clamp_box(int box) { return std::min(kMaxBox, std::max(1, box)); } bool card_has_any_tag(const Flashcard& card, @@ -273,7 +319,7 @@ void order_by_box(CardRefs* refs) { } refs->clear(); for (int box = 1; box <= kMaxBox; ++box) { - std::shuffle(boxes[box].begin(), boxes[box].end(), rng()); + deterministic_shuffle(boxes[box].begin(), boxes[box].end()); refs->insert(refs->end(), boxes[box].begin(), boxes[box].end()); } } @@ -651,7 +697,7 @@ void review_flashcards(Deck& deck) { return; } - std::shuffle(matches.begin(), matches.end(), rng()); + deterministic_shuffle(matches.begin(), matches.end()); if (session.mode == kModeBox && filters.leitner_box == 0) { order_by_box(&matches); } else if (filters.due_only) { diff --git a/tests/golden/cases/review-audio-unplayable/env b/tests/golden/cases/review-audio-unplayable/env index c1c3984..05f54cd 100644 --- a/tests/golden/cases/review-audio-unplayable/env +++ b/tests/golden/cases/review-audio-unplayable/env @@ -1,2 +1 @@ -FLASHTERM_TTS=/bin/false -FLASHTERM_PLAYER=/bin/false +FAKE_AUDIO_FAIL=1 diff --git a/tests/golden/cases/review-multi-card/expected b/tests/golden/cases/review-multi-card/expected index f8f1b67..7c3ab82 100644 --- a/tests/golden/cases/review-multi-card/expected +++ b/tests/golden/cases/review-multi-card/expected @@ -45,10 +45,10 @@ Next review in 3 days (). > Progress: [████████░░░░░░░░░░░░] 40% (2/5 cards) ┌──────────────────────────────────────────────────────────────┐ -│ Box 1 · new · french;greeting │ +│ Box 1 · new · french │ ├──────────────────────────────────────────────────────────────┤ │ │ -│ Bonjour │ +│ Au revoir │ │ │ └──────────────────────────────────────────────────────────────┘ @@ -56,14 +56,14 @@ Next review in 3 days (). Your answer: Progress: [████████░░░░░░░░░░░░] 40% (2/5 cards) ┌──────────────────────────────────────────────────────────────┐ -│ Box 1 · new · french;greeting │ +│ Box 1 · new · french │ ├──────────────────────────────────────────────────────────────┤ │ │ -│ Bonjour │ +│ Au revoir │ │ │ └──────────────────────────────────────────────────────────────┘ -Hint: H···· +Hint: G······ [Enter] submit [a] play audio [q] end session Your answer: ✅ Correct! @@ -75,16 +75,16 @@ Next review in 1 day (). > Progress: [████████████░░░░░░░░] 60% (3/5 cards) ┌──────────────────────────────────────────────────────────────┐ -│ Box 1 · new · french │ +│ Box 1 · new · french;animal │ ├──────────────────────────────────────────────────────────────┤ │ │ -│ Au revoir │ +│ Chien │ │ │ └──────────────────────────────────────────────────────────────┘ [Enter] submit [a] play audio [?] hint [q] end session Your answer: -❌ Incorrect! Correct answer: Goodbye +❌ Incorrect! Correct answer: Dog Next review in 1 day (). [Enter] next card [a] hear the question [e] edit this card @@ -110,10 +110,10 @@ Next review in 3 days (). > Progress: [████████████████████] 100% (5/5 cards) ┌──────────────────────────────────────────────────────────────┐ -│ Box 1 · new · french;animal │ +│ Box 1 · new · french;greeting │ ├──────────────────────────────────────────────────────────────┤ │ │ -│ Chien │ +│ Bonjour │ │ │ └──────────────────────────────────────────────────────────────┘ @@ -152,14 +152,14 @@ Next review in 3 days (). --- exit status --- 0 --- file deck.txt --- -Bonjour,Hello,french;greeting,0,1,1,,, +Bonjour,Hello,french;greeting,1,0,2,,, Merci,Thanks|Thank you,french,1,0,2,,, Au revoir,Goodbye,french,0,1,1,,, Chat,Cat,french;animal,1,0,2,,, -Chien,Dog,french;animal,1,0,2,,, +Chien,Dog,french;animal,0,1,1,,, --- file deck.txt.log --- ,,