Skip to content

Remove an unneeded bounds check (that logically cannot ever be violated) from the base algorithm - #713

Merged
ExplodingCabbage merged 2 commits into
masterfrom
remove-redundant-check
Oct 7, 2026
Merged

ExplodingCabbage merged 2 commits into
masterfrom
remove-redundant-check

Conversation

@ExplodingCabbage

@ExplodingCabbage ExplodingCabbage commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

There was never a need for the 0 <= addPathNewPos here:

        if (addPath) {
          // what newPos will be after we do an insertion:
          const addPathNewPos = addPath.oldPos - k;
          canAdd = addPath && 0 <= addPathNewPos && addPathNewPos < newLen;
        }

It will always be greater than 0. We start not having consumed even character 0 of the new string (newPos = -1), but the moment we add a single character, we are at newPos = 0 or more and there's no way to go back.

(And empirically, just removing this check doesn't break any tests.)

@ExplodingCabbage
ExplodingCabbage marked this pull request as ready for review October 7, 2026 21:10
@ExplodingCabbage
ExplodingCabbage requested a balanced review from Copilot October 7, 2026 21:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The maintained path invariant guarantees the removed lower bound cannot be violated.

0 open findings

What changed in this PR

Removes a redundant bounds check from the core Myers diff algorithm.

Changes:

  • Simplifies insertion-path validation while preserving the upper-bound check.
File Description
src/​diff/​base.ts Removes the logically unnecessary lower-bound calculation.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@ExplodingCabbage
ExplodingCabbage merged commit 5947609 into master Oct 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants