From 322e8c0782d11445448987f361aceec08e5d8785 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 13:43:07 -0700 Subject: [PATCH 01/13] =?UTF-8?q?Fix=20some=20bugs=20that=20cause=20minor?= =?UTF-8?q?=20annoyances=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …during the build process. Claude ferreted these out; the precipitating annoyance was the fact that `yarn install` in the Pulsar repo often somehow forces me to re-run the download-`libiconv` step. Claude doesn't know why the existing version gets removed during that process, but says that `yarn build` doesn't catch the omission because the are-we-still-fresh metadata doesn't know to check for `libiconv.2.dylib`. Also, Claude spotted some bugs in the `fetch-libiconv-61.sh` script. --- binding.gyp | 65 +++++++++++++++++++------------------ script/fetch-libiconv-61.sh | 9 ++--- 2 files changed, 39 insertions(+), 35 deletions(-) diff --git a/binding.gyp b/binding.gyp index 23943893..54d0b07b 100644 --- a/binding.gyp +++ b/binding.gyp @@ -28,27 +28,28 @@ "conditions": [ ['OS=="mac"', { "postbuilds": [ + { + 'postbuild_name': 'Copy vendored libiconv next to the binding', + # `-L` because `ext/lib/libiconv.2.dylib` is a + # symlink to the versioned dylib. We want the real + # file here — a link would still point outside of + # `build/`, which is what we're getting away from. + 'action': [ + 'cp', + '-L', + '<(module_root_dir)/ext/lib/libiconv.2.dylib', + '<(PRODUCT_DIR)/libiconv.2.dylib' + ] + }, { 'postbuild_name': 'Adjust vendored libiconv install name', 'action': [ 'install_name_tool', "-change", "libiconv.2.dylib", - "@loader_path/../../ext/lib/libiconv.2.dylib", + "@loader_path/libiconv.2.dylib", "<(PRODUCT_DIR)/superstring.node" ] - - # NOTE: This version of the post-build action - # should be used if we find it necessary to avoid - # changing the `dylib`’s install name in an earlier - # step. - # - # 'action': [ - # 'bash', - # '<(module_root_dir)/script/adjust-install-name.sh', - # '<(PRODUCT_DIR)' - # ] - } ] }] @@ -120,7 +121,7 @@ "action_name": "Run script", "message": "Building GNU libiconv...", "inputs": [], - "outputs": ["ext"], + "outputs": ["<(module_root_dir)/ext/lib/libiconv.2.dylib"], "action": [ "bash", "script/fetch-libiconv-61.sh" @@ -194,28 +195,30 @@ 'MACOSX_DEPLOYMENT_TARGET': '10.12', }, "postbuilds": [ + { + 'postbuild_name': 'Copy vendored libiconv next to the binding', + # `-L` because `ext/lib/libiconv.2.dylib` is a + # symlink to the versioned dylib. We want the real + # file here — a link would still point outside of + # `build/`, which is what we're getting away from. + 'action': [ + 'cp', + '-L', + '<(module_root_dir)/ext/lib/libiconv.2.dylib', + '<(PRODUCT_DIR)/libiconv.2.dylib' + ] + }, { 'postbuild_name': 'Adjust vendored libiconv install name', 'action': [ - 'install_name_tool', - "-change", - "libiconv.2.dylib", - "@executable_path/../../ext/lib/libiconv.2.dylib", - "<(PRODUCT_DIR)/tests" + 'install_name_tool', + "-change", + "libiconv.2.dylib", + "@executable_path/libiconv.2.dylib", + "<(PRODUCT_DIR)/tests" ] - - # NOTE: This version of the post-build action - # should be used if we find it necessary to avoid - # changing the `dylib`’s install name in an earlier - # step. - # - # 'action': [ - # 'bash', - # '<(module_root_dir)/script/adjust-install-name.sh', - # '<(PRODUCT_DIR)' - # ] } - ] + ] }] ] }] diff --git a/script/fetch-libiconv-61.sh b/script/fetch-libiconv-61.sh index 74c98904..f43cd4db 100644 --- a/script/fetch-libiconv-61.sh +++ b/script/fetch-libiconv-61.sh @@ -1,4 +1,5 @@ #!/bin/bash +set -euo pipefail # When compiling `superstring` on macOS, we used to be able to rely on the # builtin version of `libiconv`. But newer versions of macOS include FreeBSD @@ -15,7 +16,7 @@ echoerr() { echo "$@\n" >&2; } create-if-missing() { - if [ -z "$1" ]; then + if [ -f "$1" ]; then echoerr "Error: $1 is a file." usage exit 1 @@ -50,7 +51,7 @@ dylib_path="$EXT/lib/libiconv.2.dylib" # If this path already exists, we'll assume libiconv has already been fetched # and compiled. Otherwise we'll do it now. -if [ ! -L "$dylib_path" ]; then +if [ ! -e "$dylib_path" ]; then echo "Path $dylib_path is missing; fetching and installing libiconv." cd $SCRATCH # TODO: Instead of downloading this each time, we can check this into source @@ -64,7 +65,7 @@ if [ ! -L "$dylib_path" ]; then make make install - if [ ! -L "$dylib_path" ]; then + if [ ! -e "$dylib_path" ]; then echoerr "Error: expected $dylib_path to be present, but it was not. Installation of libiconv failed. Cannot proceed." usage exit 1 @@ -84,7 +85,7 @@ fi cd $ROOT # We expect this path to exist and be a symbolic link that points to a file. -if [ ! -L "$dylib_path" ]; then +if [ ! -e "$dylib_path" ]; then echoerr "Error: expected $dylib_path to be present, but it was not. Cannot proceed." usage exit 1 From 7b73c6ce27d4de73f768e7e44cfe5445571d8d4f Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 16:23:03 -0700 Subject: [PATCH 02/13] Tweak the workflow file --- .github/workflows/ci.yml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b6ae41b8..2d944ae0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,7 +1,12 @@ name: ci on: - - pull_request - - push + push: + branches: [master] + pull_request: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true jobs: Test: @@ -13,7 +18,7 @@ jobs: os: - ubuntu-latest - macos-latest - - windows-latest + - windows-2022 node_version: - 16 - 18 From 3de853447bad785f0e6ef5f3d38fc8c2b5ce8b6c Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 17:20:10 -0700 Subject: [PATCH 03/13] Further changes (pressing my luck) --- .github/workflows/ci.yml | 2 +- binding.gyp | 32 ++++---------------------------- script/copy-libiconv.sh | 24 ++++++++++++++++++++++++ script/fetch-libiconv-61.sh | 9 ++++----- 4 files changed, 33 insertions(+), 34 deletions(-) create mode 100755 script/copy-libiconv.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d944ae0..8f47253a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,9 +20,9 @@ jobs: - macos-latest - windows-2022 node_version: - - 16 - 18 - 20 + - 22 name: Node ${{ matrix.node_version }} on ${{ matrix.os }} steps: diff --git a/binding.gyp b/binding.gyp index 54d0b07b..f4b8fd80 100644 --- a/binding.gyp +++ b/binding.gyp @@ -30,13 +30,9 @@ "postbuilds": [ { 'postbuild_name': 'Copy vendored libiconv next to the binding', - # `-L` because `ext/lib/libiconv.2.dylib` is a - # symlink to the versioned dylib. We want the real - # file here — a link would still point outside of - # `build/`, which is what we're getting away from. 'action': [ - 'cp', - '-L', + 'bash', + '<(module_root_dir)/script/copy-libiconv.sh', '<(module_root_dir)/ext/lib/libiconv.2.dylib', '<(PRODUCT_DIR)/libiconv.2.dylib' ] @@ -129,22 +125,6 @@ } ] } - # { - # "target_name": "find_libiconv", - # "target_type": "none", - # "actions": [ - # { - # "action_name": "Run script", - # "message": "Locating GNU libiconv...", - # "inputs": [], - # "outputs": ["vendor/libiconv/lib/libiconv.2.dylib"], - # "action": [ - # "bash", - # "script/find-gnu-libiconv.sh" - # ] - # } - # ] - # } ] }], @@ -197,13 +177,9 @@ "postbuilds": [ { 'postbuild_name': 'Copy vendored libiconv next to the binding', - # `-L` because `ext/lib/libiconv.2.dylib` is a - # symlink to the versioned dylib. We want the real - # file here — a link would still point outside of - # `build/`, which is what we're getting away from. 'action': [ - 'cp', - '-L', + 'bash', + '<(module_root_dir)/script/copy-libiconv.sh', '<(module_root_dir)/ext/lib/libiconv.2.dylib', '<(PRODUCT_DIR)/libiconv.2.dylib' ] diff --git a/script/copy-libiconv.sh b/script/copy-libiconv.sh new file mode 100755 index 00000000..19deeb9e --- /dev/null +++ b/script/copy-libiconv.sh @@ -0,0 +1,24 @@ +#!/bin/bash +set -euo pipefail + +# Copy the vendored libiconv next to the built product. Resolves the symlink +# (`ext/lib/libiconv.2.dylib` points at the versioned dylib) and installs the +# copy atomically, so two targets postbuilding in parallel can't observe a +# half-written file. + +src="$1" +dest="$2" + +tmp="$(mktemp "${dest}.XXXXXX")" +trap 'rm -f "$tmp"' EXIT + +# `-L` because `ext/lib/libiconv.2.dylib` is a symlink to the versioned dylib. +# We want the real file here — a link would still point outside of `build/`, +# which is what we're getting away from. +cp -L "$src" "$tmp" + +# We _must_ get the permissions right here; a `.dylib` with 0600 would work for +# the user who built it but fail for anyone else. +chmod 755 "$tmp" + +mv -f "$tmp" "$dest" diff --git a/script/fetch-libiconv-61.sh b/script/fetch-libiconv-61.sh index f43cd4db..6cee8b5c 100644 --- a/script/fetch-libiconv-61.sh +++ b/script/fetch-libiconv-61.sh @@ -13,7 +13,7 @@ set -euo pipefail # `libiconv.2.dylib`. For now, letting the user compile their own `libiconv` # has the advantage of very likely matching the system's architecture. -echoerr() { echo "$@\n" >&2; } +echoerr() { printf '%s\n\n' "$*" >&2; } create-if-missing() { if [ -f "$1" ]; then @@ -22,7 +22,7 @@ create-if-missing() { exit 1 fi if [ ! -d "$1" ]; then - mkdir "$1" + mkdir -p "$1" fi } @@ -53,7 +53,7 @@ dylib_path="$EXT/lib/libiconv.2.dylib" # and compiled. Otherwise we'll do it now. if [ ! -e "$dylib_path" ]; then echo "Path $dylib_path is missing; fetching and installing libiconv." - cd $SCRATCH + cd "$SCRATCH" # TODO: Instead of downloading this each time, we can check this into source # control via git subtree. That would allow someone to build this without # needing internet connectivity. But we'd still need to do a `make install` — @@ -82,9 +82,8 @@ else echo "Path $dylib_path is already present; skipping installation of libiconv." fi -cd $ROOT +cd "$ROOT" -# We expect this path to exist and be a symbolic link that points to a file. if [ ! -e "$dylib_path" ]; then echoerr "Error: expected $dylib_path to be present, but it was not. Cannot proceed." usage From 5924dfe5dda46da135b276d503d30964333e00a3 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Mon, 7 Sep 2026 18:00:57 -0700 Subject: [PATCH 04/13] Bump service versions --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8f47253a..973f7476 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,18 +26,18 @@ jobs: name: Node ${{ matrix.node_version }} on ${{ matrix.os }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: submodules: true - name: Cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: | node_modules key: ${{ runner.os }}-${{ matrix.node_version }}-${{ hashFiles('package.json') }} - name: Setup node - uses: actions/setup-node@v4 + uses: actions/setup-node@v5 with: node-version: ${{ matrix.node_version }} From c93a2154ae7c167171af76906dcd4c235a34724a Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 15:08:50 -0700 Subject: [PATCH 05/13] Bump Node version; other small fixes --- .github/workflows/ci.yml | 2 +- .github/workflows/publish.yml | 2 +- .npmignore | 1 + binding.gyp | 2 +- package.json | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 973f7476..9c3e3e49 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,7 +5,7 @@ on: pull_request: concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true jobs: diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 36c30859..4321c920 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -6,7 +6,7 @@ on: workflow_dispatch: env: - NODE_VERSION: 16 + NODE_VERSION: 20 NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISH_TOKEN }} jobs: diff --git a/.npmignore b/.npmignore index 6d32e24c..713f0bb9 100644 --- a/.npmignore +++ b/.npmignore @@ -9,6 +9,7 @@ !src/bindings/*.cc !script/fetch-libiconv-61.sh +!script/copy-libiconv.sh !vendor/libcxx/* diff --git a/binding.gyp b/binding.gyp index f4b8fd80..955224cc 100644 --- a/binding.gyp +++ b/binding.gyp @@ -194,7 +194,7 @@ "<(PRODUCT_DIR)/tests" ] } - ] + ] }] ] }] diff --git a/package.json b/package.json index 969859b0..5fc91f68 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "data-structure" ], "engines": { - "node": ">=16" + "node": ">=18" }, "author": "Nathan Sobo ", "license": "MIT", From a7685f66bf7479cd354ffdf564c87dfd55633038 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 15:09:11 -0700 Subject: [PATCH 06/13] Test theory about crashing --- test/js/marker-index.test.js | 52 ++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/js/marker-index.test.js b/test/js/marker-index.test.js index b4f8d38e..eea04b0f 100644 --- a/test/js/marker-index.test.js +++ b/test/js/marker-index.test.js @@ -534,4 +534,56 @@ describe('MarkerIndex', () => { let result = index.findEndingIn({row: 0, column: 0}, {row: Infinity, column: Infinity}) assert(result.has(1)) }) + + // `MarkerIndex::remove` dereferences the result of `unordered_map::find` + // without checking it against `end()`. For an id the index doesn't hold, + // libc++ and libstdc++ represent `end()` as a null node, so these calls + // segfault immediately; MSVC represents it as a live list sentinel whose + // value is never constructed, so on Windows the garbage read is silent and + // the process dies later, while `~MarkerIndex` tears the map down. + // + // These crash the process rather than throwing, so a failure here takes the + // whole mocha run down with it. + describe('remove with an unknown id', () => { + it('ignores an id that was never inserted', () => { + let index = new MarkerIndex(1) + index.insert(1, {row: 0, column: 0}, {row: 0, column: 5}) + + index.remove(999) + + assert.isTrue(index.has(1)) + assert.deepEqual(index.getRange(1), {start: {row: 0, column: 0}, end: {row: 0, column: 5}}) + }) + + it('ignores a second removal of the same id', () => { + let index = new MarkerIndex(1) + index.insert(1, {row: 0, column: 0}, {row: 0, column: 5}) + + index.remove(1) + index.remove(1) + + assert.isFalse(index.has(1)) + }) + + it('ignores a removal from an empty index', () => { + let index = new MarkerIndex(1) + + index.remove(1) + + assert.isFalse(index.has(1)) + }) + + it('leaves surrounding markers intact after a no-op removal', () => { + let index = new MarkerIndex(1) + index.insert(1, {row: 0, column: 0}, {row: 0, column: 5}) + index.insert(2, {row: 1, column: 0}, {row: 1, column: 5}) + + index.remove(999) + index.remove(1) + + assert.isFalse(index.has(1)) + assert.isTrue(index.has(2)) + assert.deepEqual(index.getRange(2), {start: {row: 1, column: 0}, end: {row: 1, column: 5}}) + }) + }) }) From c2542e6be70f614ee2c24bafbc01d14c12832f05 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 15:36:27 -0700 Subject: [PATCH 07/13] Un-break test-native on Windows --- .github/workflows/ci.yml | 14 ++++++++++---- script/test-native.js | 19 ++++++++++++++++--- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c3e3e49..c423ca20 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,10 +72,16 @@ jobs: - name: Lint run: npm run standard - - name: Run tests - run: | - npm run test:node - npm run test:native + # These are deliberately separate steps. On Windows the default shell is + # `pwsh`, where a multi-line `run:` block neither halts on a failed native + # command nor reports anything but the last command's exit code — so a + # crash in `test:node` was silently discarded by `test:native` running + # after it. + - name: Run JS tests + run: npm run test:node + + - name: Run native tests + run: npm run test:native Skip: if: contains(github.event.head_commit.message, '[skip ci]') diff --git a/script/test-native.js b/script/test-native.js index ae218946..eb964816 100755 --- a/script/test-native.js +++ b/script/test-native.js @@ -4,7 +4,10 @@ const fs = require('fs') const path = require('path') const {spawnSync} = require('child_process') -const testsPath = path.resolve(__dirname, '..', 'build', 'Debug', 'tests') +const isWindows = process.platform === 'win32' +const testsPath = path.resolve( + __dirname, '..', 'build', 'Debug', isWindows ? 'tests.exe' : 'tests' +) const dotPath = path.resolve(__dirname, '..', 'build', 'debug.dot') const htmlPath = path.join(__dirname, '..', 'build', 'debug.html') @@ -52,6 +55,16 @@ switch (args[0]) { } function run(command, args = [], options = {stdio: 'inherit'}) { - const {status} = spawnSync(command, args, options) - if (status !== 0) process.exit(status) + // `shell` is needed on Windows so that `node-gyp` resolves to `node-gyp.cmd`; + // CreateProcess cannot launch a batch file directly. + const {status, error} = spawnSync(command, args, {shell: isWindows, ...options}) + + // A failure to spawn at all reports `status: null`, which is not `0` — so the + // old check handed it to `process.exit`, where Node coerced it to 0 and the + // run was reported as a success. + if (error) { + console.error(`Failed to run ${command}: ${error.message}`) + process.exit(1) + } + if (status !== 0) process.exit(status === null ? 1 : status) } \ No newline at end of file From 97363558b8a9556041f8715356d25b93b206ee2e Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 15:48:19 -0700 Subject: [PATCH 08/13] Fix unchecked map read --- src/core/marker-index.cc | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/core/marker-index.cc b/src/core/marker-index.cc index d622c7ec..7bc8cba7 100644 --- a/src/core/marker-index.cc +++ b/src/core/marker-index.cc @@ -464,8 +464,21 @@ void MarkerIndex::set_exclusive(MarkerId id, bool exclusive) { } void MarkerIndex::remove(MarkerId id) { - Node *start_node = start_nodes_by_id.find(id)->second; - Node *end_node = end_nodes_by_id.find(id)->second; + // Both maps are populated and cleared together, but check each one anyway: + // `splice` reassigns them independently, so treat either miss as "we don't + // hold this marker." Dereferencing an `end()` iterator here is undefined — + // libc++ and libstdc++ represent it as a null node and crash immediately, + // while MSVC represents it as a live list sentinel whose value was never + // constructed, quietly yielding garbage that we then write through and free. + auto start_entry = start_nodes_by_id.find(id); + auto end_entry = end_nodes_by_id.find(id); + if (start_entry == start_nodes_by_id.end() || + end_entry == end_nodes_by_id.end()) { + return; + } + + Node *start_node = start_entry->second; + Node *end_node = end_entry->second; Node *node = start_node; while (node) { From 60029d9f85d66946ff2ca148cca6abaa30f9ec72 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 16:31:24 -0700 Subject: [PATCH 09/13] Fix a test failure caused by an invalid boundary between CR and LF --- src/core/text-buffer.cc | 12 ++++++++++++ test/native/test-helpers.cc | 11 ++++++++++- test/native/test-helpers.h | 7 +++++++ test/native/text-buffer-test.cc | 9 +++++---- test/native/text-diff-test.cc | 9 +++++---- 5 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/core/text-buffer.cc b/src/core/text-buffer.cc index c4dd3e8c..3be8bb52 100644 --- a/src/core/text-buffer.cc +++ b/src/core/text-buffer.cc @@ -373,6 +373,18 @@ struct TextBuffer::Layer { slice_to_search_start_position.traverse(match_end_position) }; + // A match can begin on the LF of a CRLF pair whose CR lives in an + // earlier chunk. `position_for_offset` only ever looks within a + // single chunk, so it cannot see the CR and cannot clip the start + // itself. Points within CRLF line endings are not valid, so back + // the start up onto the CR here — the same adjustment made for + // match ends at a chunk boundary above, but for the other end. + if (last_match.start.column > 0 && + character_at(last_match.start) == '\n' && + character_at(previous_column(last_match.start)) == '\r') { + last_match.start.column--; + } + last_search_end_position = last_match.end; if (match_end_position == match_start_position) { last_search_end_position.column++; diff --git a/test/native/test-helpers.cc b/test/native/test-helpers.cc index dd78dc87..2544a1c2 100644 --- a/test/native/test-helpers.cc +++ b/test/native/test-helpers.cc @@ -4,6 +4,8 @@ #include "text-buffer.h" #include #include +#include +#include #include #include #include @@ -36,7 +38,7 @@ std::unique_ptr get_text(const u16string content) { std::u16string get_random_string(Generator &rand, uint32_t character_count) { u16string content; content.reserve(character_count); - for (uint i = 0; i < character_count; i++) { + for (uint32_t i = 0; i < character_count; i++) { if (rand() % 20 < 1) { content.push_back('\n'); } else if (rand() % 20 < 1) { @@ -73,3 +75,10 @@ Range get_random_range(Generator &rand, const Text &text) { Range get_random_range(Generator &rand, TextBuffer &buffer) { return get_random_range(rand, buffer.text()); } + +uint32_t get_seed_base() { + if (const char *seed = getenv("SUPERSTRING_TEST_SEED")) { + return static_cast(strtoul(seed, nullptr, 10)); + } + return static_cast(time(nullptr) * 1000); +} diff --git a/test/native/test-helpers.h b/test/native/test-helpers.h index 53d83244..99465613 100644 --- a/test/native/test-helpers.h +++ b/test/native/test-helpers.h @@ -34,6 +34,13 @@ Text get_random_text(Generator &); Range get_random_range(Generator &, const Text &); Range get_random_range(Generator &, TextBuffer &); +// Base seed for the randomized test loops. Set SUPERSTRING_TEST_SEED to replay +// a specific failure; otherwise it comes from the clock so each run explores +// new scenarios. Seeds are NOT portable across platforms or standard library +// versions — `default_random_engine` and `uniform_int_distribution` are both +// implementation-defined, so the same seed yields different data elsewhere. +uint32_t get_seed_base(); + namespace std { inline std::ostream &operator<<(std::ostream &stream, const std::u16string &text) { for (uint16_t character : text) { diff --git a/test/native/text-buffer-test.cc b/test/native/text-buffer-test.cc index d0ca5fba..5c996c8d 100644 --- a/test/native/text-buffer-test.cc +++ b/test/native/text-buffer-test.cc @@ -500,9 +500,10 @@ void query_random_ranges(TextBuffer &buffer, Generator &rand, Text &mutated_text TEST_CASE("TextBuffer - random edits and queries") { TextBuffer::MAX_CHUNK_SIZE_TO_COPY = 2; - auto t = time(nullptr); - for (uint i = 0; i < 100; i++) { - uint32_t seed = t * 1000 + i; + auto t = get_seed_base(); + for (uint32_t i = 0; i < 100; i++) { + uint32_t seed = t + i; + CAPTURE(seed); Generator rand(seed); cout << "seed: " << seed << "\n"; @@ -516,7 +517,7 @@ TEST_CASE("TextBuffer - random edits and queries") { // cout << "edit: " << i << "\n"; // cout << "extent: " << original_text.extent() << "\ntext: " << original_text << "\n"; - for (uint j = 0; j < 15; j++) { + for (uint32_t j = 0; j < 15; j++) { // cout << "iteration: " << j << "\n"; Text mutated_text = buffer.text(); diff --git a/test/native/text-diff-test.cc b/test/native/text-diff-test.cc index 6dc79abd..aaa6e9c5 100644 --- a/test/native/text-diff-test.cc +++ b/test/native/text-diff-test.cc @@ -105,9 +105,10 @@ TEST_CASE("text_diff - old text is a suffix of new text") { } TEST_CASE("text_diff - randomized changes") { - auto t = time(nullptr); - for (uint i = 0; i < 100; i++) { - uint32_t seed = t * 1000 + i; + auto t = get_seed_base(); + for (uint32_t i = 0; i < 100; i++) { + uint32_t seed = t + i; + CAPTURE(seed); Generator rand(seed); cout << "seed: " << seed << "\n"; @@ -116,7 +117,7 @@ TEST_CASE("text_diff - randomized changes") { // cout << "extent: " << new_text.extent() << " text:\n" << new_text << "\n\n"; - for (uint j = 0; j < 1 + rand() % 10; j++) { + for (uint32_t j = 0; j < 1 + rand() % 10; j++) { // cout << "j: " << j << "\n"; Range deleted_range = get_random_range(rand, new_text); From 93d587b826efb8f4cd0e7aff71a5cdd362ff9c42 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 16:38:29 -0700 Subject: [PATCH 10/13] Fix malformed strings --- test/native/encoding-conversion-test.cc | 8 ++++---- test/native/text-buffer-test.cc | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/native/encoding-conversion-test.cc b/test/native/encoding-conversion-test.cc index 992c99ef..979b9b85 100644 --- a/test/native/encoding-conversion-test.cc +++ b/test/native/encoding-conversion-test.cc @@ -67,7 +67,7 @@ TEST_CASE("EncodingConversion::decode - four-byte UTF-16 characters") { u16string string; conversion->decode(string, input.data(), input.size()); - REQUIRE(string == u"ab" "\xd83d" "\xde01" "cd"); + REQUIRE(string == u"ab" u"\xd83d" u"\xde01" u"cd"); } TEST_CASE("EncodingConversion::encode - basic") { @@ -93,7 +93,7 @@ TEST_CASE("EncodingConversion::encode - basic") { TEST_CASE("EncodingConversion::encode - four-byte UTF-16 characters") { auto conversion = transcoding_to("UTF-8"); - u16string string = u"ab" "\xd83d" "\xde01" "cd"; // 'ab😁cd' + u16string string = u"ab" u"\xd83d" u"\xde01" u"cd"; // 'ab😁cd' vector output(10); size_t bytes_encoded = 0, start = 0; @@ -116,7 +116,7 @@ TEST_CASE("EncodingConversion::encode - four-byte UTF-16 characters") { TEST_CASE("EncodingConversion::encode - invalid characters in the middle of the string") { auto conversion = transcoding_to("UTF-8"); - u16string string = u"abc" "\xD800" "def"; + u16string string = u"abc" u"\xD800" u"def"; vector output(10); size_t bytes_encoded = 0, start = 0; @@ -136,7 +136,7 @@ TEST_CASE("EncodingConversion::encode - invalid characters in the middle of the TEST_CASE("EncodingConversion::encode - invalid characters at the end of the string") { auto conversion = transcoding_to("UTF-8"); - u16string string = u"abc" "\xD800"; + u16string string = u"abc" u"\xD800"; vector output(10); size_t bytes_encoded = 0, start = 0; diff --git a/test/native/text-buffer-test.cc b/test/native/text-buffer-test.cc index 5c996c8d..d55ab563 100644 --- a/test/native/text-buffer-test.cc +++ b/test/native/text-buffer-test.cc @@ -466,7 +466,7 @@ TEST_CASE("TextBuffer::find_words_with_subsequence_in_range") { } TEST_CASE("TextBuffer::has_astral") { - REQUIRE(TextBuffer{u"ab" "\xd83d" "\xde01" "cd"}.has_astral()); + REQUIRE(TextBuffer{u"ab" u"\xd83d" u"\xde01" u"cd"}.has_astral()); REQUIRE(!TextBuffer{u"abcd"}.has_astral()); } From 5a674ca3a40c57082137e0462cd34de9da1c9139 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 17:14:07 -0700 Subject: [PATCH 11/13] More Windows-specific fixes --- binding.gyp | 9 +++++++++ test/native/encoding-conversion-test.cc | 6 +++--- test/native/text-buffer-test.cc | 5 +++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/binding.gyp b/binding.gyp index 955224cc..e9e5f8a5 100644 --- a/binding.gyp +++ b/binding.gyp @@ -221,6 +221,15 @@ "defines": [ "NOMINMAX" ], + # These sources are UTF-8. Absent this flag — and absent a BOM, + # which none of them have — MSVC decodes them using the system + # ANSI codepage, which mangles non-ASCII characters in string + # literals without any diagnostic. + "msvs_settings": { + "VCCLCompilerTool": { + "AdditionalOptions": ["/utf-8"] + } + }, }] ] } diff --git a/test/native/encoding-conversion-test.cc b/test/native/encoding-conversion-test.cc index 979b9b85..17e3d140 100644 --- a/test/native/encoding-conversion-test.cc +++ b/test/native/encoding-conversion-test.cc @@ -33,7 +33,7 @@ TEST_CASE("EncodingConversion::decode - basic ISO-8859-1") { u16string string; conversion->decode(string, input.data(), input.size()); - REQUIRE(string == u"qrstüv"); + REQUIRE(string == u"qrst" u"\x00fc" u"v"); // qrstüv } TEST_CASE("EncodingConversion::decode - invalid byte sequences in the middle of the input") { @@ -42,7 +42,7 @@ TEST_CASE("EncodingConversion::decode - invalid byte sequences in the middle of u16string string; conversion->decode(string, input.data(), input.size()); - REQUIRE(string == u"ab" "\ufffd" "\ufffd" "de"); + REQUIRE(string == u"ab" u"\ufffd" u"\ufffd" u"de"); } TEST_CASE("EncodingConversion::decode - invalid byte sequences at the end of the input") { @@ -58,7 +58,7 @@ TEST_CASE("EncodingConversion::decode - invalid byte sequences at the end of the string.clear(); bytes_encoded = conversion->decode(string, input.data(), input.size(), true); REQUIRE(bytes_encoded == 4); - REQUIRE(string == u"ab" "\ufffd" "\ufffd"); + REQUIRE(string == u"ab" u"\ufffd" u"\ufffd"); } TEST_CASE("EncodingConversion::decode - four-byte UTF-16 characters") { diff --git a/test/native/text-buffer-test.cc b/test/native/text-buffer-test.cc index d55ab563..82894665 100644 --- a/test/native/text-buffer-test.cc +++ b/test/native/text-buffer-test.cc @@ -4,7 +4,8 @@ #include "text-slice.h" #include "regex.h" #include -#include +#include +#include using std::move; using std::pair; @@ -536,7 +537,7 @@ TEST_CASE("TextBuffer - random edits and queries") { Generator rand(seed); vector results; for (uint32_t k = 0; k < 5; k++) { - usleep(rand() % 1000); + std::this_thread::sleep_for(std::chrono::microseconds(rand() % 1000)); vector line_ending_positions; for (uint32_t row = 0; row < snapshot->extent().row; row++) { line_ending_positions.push_back({row, snapshot->line_length_for_row(row)}); From 1e534d38914ea568437e5f1f113b6d5dee995765 Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 17:26:58 -0700 Subject: [PATCH 12/13] Disambiguate `==` in `optional == optional` --- src/core/optional.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/core/optional.h b/src/core/optional.h index 61280d99..2f8370bc 100644 --- a/src/core/optional.h +++ b/src/core/optional.h @@ -16,8 +16,13 @@ template class optional { const T &operator*() const { return value; } const T *operator->() const { return &value; } T *operator->() { return &value; } - operator bool() const { return is_some; } - bool operator==(const optional &other) { + // Deliberately `explicit`: an implicit conversion to bool makes + // `optional == optional` ambiguous, because the built-in `bool == + // bool` becomes as good a candidate as the member below. Clang and GCC pick + // the member anyway; MSVC rejects the comparison outright (C2666). Every + // contextual use — `if (x)`, `!x`, `x && y`, `x ? a : b` — still works. + explicit operator bool() const { return is_some; } + bool operator==(const optional &other) const { if (is_some) { return other.is_some && value == other.value; } else { From 36397dbc3f3e6bac8ac4e701decd374dee65b1de Mon Sep 17 00:00:00 2001 From: Andrew Dupont Date: Sat, 19 Sep 2026 17:57:41 -0700 Subject: [PATCH 13/13] Don't add --debug on Windows for test-native --- script/test-native.js | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/script/test-native.js b/script/test-native.js index eb964816..97f2734c 100755 --- a/script/test-native.js +++ b/script/test-native.js @@ -5,16 +5,35 @@ const path = require('path') const {spawnSync} = require('child_process') const isWindows = process.platform === 'win32' + +// MSVC's Debug configuration enables checked iterators +// (`_ITERATOR_DEBUG_LEVEL=2`) and `/RTC1` runtime checks. For a suite this +// STL-heavy that is orders of magnitude slower — tens of minutes on CI, versus +// under two seconds elsewhere. Clang's `-O0` does none of that, so Debug stays +// the default on other platforms, where it costs nothing and keeps the binary +// friendly to `lldb`. +// +// This does not weaken what CI checks: node-gyp never defines `NDEBUG`, so the +// `assert()` calls throughout `src/core` stay live in Release too. +// +// Set SUPERSTRING_TEST_CONFIG to 'Debug' or 'Release' to override — you want +// 'Debug' if you're about to attach a debugger on Windows. +const configuration = process.env.SUPERSTRING_TEST_CONFIG || + (isWindows ? 'Release' : 'Debug') +const isDebug = configuration === 'Debug' + const testsPath = path.resolve( - __dirname, '..', 'build', 'Debug', isWindows ? 'tests.exe' : 'tests' + __dirname, '..', 'build', configuration, isWindows ? 'tests.exe' : 'tests' ) const dotPath = path.resolve(__dirname, '..', 'build', 'debug.dot') const htmlPath = path.join(__dirname, '..', 'build', 'debug.html') if (fs.existsSync(testsPath)) { - run('node-gyp', ['build']) + run('node-gyp', isDebug ? ['build', '--debug'] : ['build']) } else { - run('node-gyp', ['rebuild', '--debug', '--tests']) + run('node-gyp', isDebug + ? ['rebuild', '--debug', '--tests'] + : ['rebuild', '--tests']) } const args = process.argv.slice(2)