◐ Shell
clean mode source ↗

prefer nb_bool slot in try_to_bool instead of __bool__ by YashSuthar983 · Pull Request #6328 · RustPython/RustPython

YashSuthar983

Caution

Review failed

The pull request is closed.

Walkthrough

Prioritizes a precomputed numeric boolean slot (as_number.boolean) in try_to_bool, falling back to __bool__ descriptor and then to __len__ if needed. Preserves existing error propagation and type checks for non-bool or negative __len__ results.

Changes

Cohort / File(s) Summary
Boolean slot optimization
crates/vm/src/builtins/bool.rs		 Added a fast-path that uses the as_number.boolean slot when present to obtain truthiness directly; retained fallback to __bool__ descriptor (with error propagation and type validation) and then to __len__ (requiring non-negative integer).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to:
    • crates/vm/src/builtins/bool.rs control-flow around slot vs descriptor resolution.
    • Preservation of exact error messages for non-bool __bool__ returns and negative __len__.
    • Correct extraction of raw boolean value via get_value and interactions with AsNumber layout.

Possibly related PRs

Poem

🐇
I nibble slots where booleans hide,
A fast-path hop, no methods tried.
If descriptors wake, I'll step aside,
Else count the length and decide—
Hooray, truth found with bunny pride! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main optimization: preferring the nb_bool slot over bool in try_to_bool, which aligns with the changeset's primary objective.
Linked Issues check ✅ Passed The PR implements the required objective from #6113: updating try_to_bool to prefer nb_bool (number slot boolean) over calling bool for better performance.
Out of Scope Changes check ✅ Passed All changes are focused on the try_to_bool optimization within bool.rs and directly address the linked issue requirement; no out-of-scope changes detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 628f59e and 9e6591e.

📒 Files selected for processing (1)
  • crates/vm/src/builtins/bool.rs (1 hunks)

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

Comment @coderabbitai help to get the list of available commands and usage tips.