Check blockidx out of range for dense UniTensor#701
Check blockidx out of range for dense UniTensor#701manuschneider wants to merge 3 commits intomasterfrom
Conversation
… any argument to avoid overhead of argument check; did not do this for put_block(_) for code readability
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 32.74% 35.47% +2.72%
==========================================
Files 215 215
Lines 35065 33089 -1976
Branches 14070 13175 -895
==========================================
+ Hits 11482 11737 +255
+ Misses 21708 19428 -2280
- Partials 1875 1924 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Posted by Claude Code on behalf of @pcchen
Overall: The fix is correct in spirit and the tests are properly un-gated. There is one critical bug and a couple of minor issues.
Critical Bug — Wrong method name in put_block_ error message
In put_block_, the new error message reads:
"invalid. Use get_block_(0).\n"
But it is inside put_block_, so it should say "invalid. Use put_block_(0).\n". This is a copy-paste error from the get_block_ implementation.
Minor — %d format specifier for cytnx_uint64
All four new cytnx_error_msg calls use %d to print a cytnx_uint64 value. Since cytnx_uint64 is an unsigned 64-bit integer, this is technically undefined behavior — %lu or %llu should be used. (This may be a pre-existing codebase-wide pattern, but worth fixing here since you're touching it.)
Minor — API inconsistency between get_block and put_block
get_block/get_block_ are split into two overloads (no-arg fast path + arg-with-check), but put_block/put_block_ remain a single function with the check inline. The commit message explains this was intentional ("for code readability"), but it creates an inconsistent API. Users looking at the put_block signature will still see idx = 0 as a default and may not expect an error. Consider applying the same two-overload pattern for consistency.
Positive
- Core logic (
idx != 0guard) is correct for all four methods. cytnx_error_msgthrowsstd::logic_error(confirmed incytnx_error.hpp:46), so the tests'EXPECT_THROW(..., std::logic_error)will pass.- Removing the
#if FAIL_CASE_OPENguards properly enables the previously-skipped tests. - Typo fix "convinent" → "convenient" is good.
…ifier - Fix copy-paste bug: put_block_ error message incorrectly said "Use get_block_(0)" instead of "Use put_block_(0)" - Replace %d with %llu (and cast to unsigned long long) for cytnx_uint64 in all four new error messages (get_block, get_block_ x2, put_block, put_block_) to avoid undefined behavior - Apply the same two-overload pattern used by get_block/get_block_ to put_block/put_block_: no-arg fast path + idx-with-check overload that delegates to it, making the API consistent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Fixes #176 and #298