Skip to content

No alloc in lookup_alias/lookup_named_sequence - #9819

Merged
youknowone merged 1 commit into
RustPython:mainfrom
joshuamegnauth54:no-alloc-lookup-alias
Nov 13, 2026
Merged

youknowone merged 1 commit into
RustPython:mainfrom
joshuamegnauth54:no-alloc-lookup-alias

Conversation

@joshuamegnauth54

@joshuamegnauth54 joshuamegnauth54 commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

The original code allocated for case insensitive comparison. Instead, I iterate through both strings' bytes and convert the bytes to uppercase for the comparison. This has the added benefit of only iterating once in total. The old code iterated at least three times (once per each string pair and then a third time to compare).

  • Closes #xxxx

One of checkbox below must be checked.

  • I did not use AI to write the code of this patch.
  • This PR follows our AI policy

Summary by CodeRabbit

  • Refactor
    • Unicode name and alias comparisons now avoid creating temporary uppercase strings during lookup. Case-insensitive matching behavior remains unchanged.

The original code allocated for case insensitive comparison. Instead, I
iterate through both strings' bytes and convert the bytes to uppercase
for the comparison. This has the added benefit of only iterating once in
total. The old code iterated at least three times (once per each string
pair and then a third time to compare).
@chatgpt-codex-connector

Copy link
Copy Markdown
Codex Review Summary: ❌ Failed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ❌ Failed 2026-11-12T17:59:55.023697Z
We couldn’t complete this review. Comment @codex review to try again.
0d005a6 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Alias and named-sequence binary searches now compare names with ASCII-uppercased byte iterators. They no longer build uppercased String keys.

Changes: Unicode name lookups

Layer / File(s) Summary
Compare names without allocating folded strings
crates/unicode/src/data.rs
cmp_caseless compares ASCII-uppercased bytes. Alias and named-sequence binary searches use this comparison. The searches retain case-insensitive ASCII ordering.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Refactor

Suggested reviewers: youknowone

Merge Risk: Minimal · up to 0d005

This change removes allocations from alias and named-sequence lookups without altering the ordering contract. No actionable merge risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "No alloc in lookup_alias/lookup_named_sequence" names the two functions whose case-insensitive comparison changed. The raw summary says the binary searches now compare ASCII-uppercased byte…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "No alloc in lookup_alias/lookup_named_sequence" names the two functions whose case-insensitive comparison changed. The raw summary says the binary searches now compare ASCII-uppercased byte iterators directly instead of building an uppercased String key, which removes the allocation the title describes. The title states the main goal and names the affected functions, so it fully matches the changeset.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between dba7a05 and 0d005a6.

Files selected for processing: 1

  • crates/unicode/src/data.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@youknowone
youknowone merged commit f1cc68e into RustPython:main Nov 13, 2026
20 checks 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