Disable broken/unsafe Scalar operations [also a bit of memory corruption]#937
Disable broken/unsafe Scalar operations [also a bit of memory corruption]#937ianmccul wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Scalar class to utilize templates and C++20 concepts, significantly reducing code duplication across various data types. It also introduces strict validation checks to prevent unsupported arithmetic and comparison operations on mixed signedness or boolean types, and adds a comprehensive unit test suite. The review feedback highlights a potential out-of-bounds array access when initializing cv-qualified types, recommends implementing an RAII guard for Google Test's stream capturing to prevent state leaks, and suggests refactoring self-assignment tests to avoid compiler warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| template <CytnxType T> | ||
| void Init_by_number(const T &in) { | ||
| if (this->_impl != nullptr) delete this->_impl; | ||
| this->_impl = new Int16Scalar(in); | ||
| } | ||
| void Init_by_number(const cytnx_uint16 &in) { | ||
| if (this->_impl != nullptr) delete this->_impl; | ||
| this->_impl = new Uint16Scalar(in); | ||
| } | ||
| void Init_by_number(const cytnx_bool &in) { | ||
| if (this->_impl != nullptr) delete this->_impl; | ||
| this->_impl = new BoolScalar(in); | ||
| this->_impl = __ScII.UScIInit[Type.cy_typeid(in)](); | ||
| this->_impl->assign_selftype(in); | ||
| } |
There was a problem hiding this comment.
Using Type.cy_typeid(in) to index __ScII.UScIInit can lead to out-of-bounds array access (undefined behavior or crash) if T is a cv-qualified type (such as volatile cytnx_double). The CytnxType concept allows cv-qualified types because it uses std::remove_cvref_t, but Type.cy_typeid does not strip cv-qualifiers, resulting in an index of N_Type (12) which is out of bounds for UScIInit (size 12). Use Type.cy_typeid_v<std::remove_cvref_t<T>> instead to resolve the correct index safely at compile time.
template <CytnxType T>
void Init_by_number(const T &in) {
if (this->_impl != nullptr) delete this->_impl;
this->_impl = __ScII.UScIInit[Type.cy_typeid_v<std::remove_cvref_t<T>>]();
this->_impl->assign_selftype(in);
}There was a problem hiding this comment.
In the statement const T, the base type T will never be const-qualified. It could be volatile I guess, but really?!?!
The proper fix is that constructing cy_typeid_v<T> on an invalid T should not compile. I'm very surprised that you think it does .... ARGH ... another PR coming ...
| template <typename Fn> | ||
| void ExpectCorrectDoubleOrDisabled(Fn &&fn, double expected) { | ||
| testing::internal::CaptureStdout(); | ||
| testing::internal::CaptureStderr(); | ||
| try { | ||
| const Scalar value = fn(); | ||
| static_cast<void>(testing::internal::GetCapturedStderr()); | ||
| static_cast<void>(testing::internal::GetCapturedStdout()); | ||
| ExpectDoubleScalarEq(value, expected); | ||
| } catch (const std::logic_error &) { | ||
| static_cast<void>(testing::internal::GetCapturedStderr()); | ||
| static_cast<void>(testing::internal::GetCapturedStdout()); | ||
| SUCCEED(); | ||
| } | ||
| } | ||
|
|
||
| template <typename Fn> | ||
| void ExpectCorrectInt64OrDisabled(Fn &&fn, cytnx_int64 expected) { | ||
| testing::internal::CaptureStdout(); | ||
| testing::internal::CaptureStderr(); | ||
| try { | ||
| const Scalar value = fn(); | ||
| static_cast<void>(testing::internal::GetCapturedStderr()); | ||
| static_cast<void>(testing::internal::GetCapturedStdout()); | ||
| EXPECT_EQ(static_cast<cytnx_int64>(value), expected); | ||
| } catch (const std::logic_error &) { | ||
| static_cast<void>(testing::internal::GetCapturedStderr()); | ||
| static_cast<void>(testing::internal::GetCapturedStdout()); | ||
| SUCCEED(); | ||
| } | ||
| } | ||
|
|
||
| template <typename Fn> | ||
| void ExpectDisabled(Fn &&fn) { | ||
| testing::internal::CaptureStdout(); | ||
| testing::internal::CaptureStderr(); | ||
| try { | ||
| static_cast<void>(fn()); | ||
| static_cast<void>(testing::internal::GetCapturedStderr()); | ||
| static_cast<void>(testing::internal::GetCapturedStdout()); | ||
| FAIL() << "Expected std::logic_error."; | ||
| } catch (const std::logic_error &) { | ||
| static_cast<void>(testing::internal::GetCapturedStderr()); | ||
| static_cast<void>(testing::internal::GetCapturedStdout()); | ||
| SUCCEED(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The helper functions ExpectCorrectDoubleOrDisabled, ExpectCorrectInt64OrDisabled, and ExpectDisabled capture stdout and stderr using Google Test's internal APIs. However, if an unexpected exception is thrown, or if FAIL() is executed (which returns early from the function), the captured streams are never released. This leaks the captured state and breaks the output of all subsequent tests in the suite. Implementing a simple RAII CaptureGuard ensures that the captured streams are always safely released when the guard goes out of scope.
struct CaptureGuard {
CaptureGuard() {
testing::internal::CaptureStdout();
testing::internal::CaptureStderr();
}
~CaptureGuard() {
static_cast<void>(testing::internal::GetCapturedStdout());
static_cast<void>(testing::internal::GetCapturedStderr());
}
};
template <typename Fn>
void ExpectCorrectDoubleOrDisabled(Fn &&fn, double expected) {
CaptureGuard guard;
try {
const Scalar value = fn();
ExpectDoubleScalarEq(value, expected);
} catch (const std::logic_error &) {
SUCCEED();
}
}
template <typename Fn>
void ExpectCorrectInt64OrDisabled(Fn &&fn, cytnx_int64 expected) {
CaptureGuard guard;
try {
const Scalar value = fn();
EXPECT_EQ(static_cast<cytnx_int64>(value), expected);
} catch (const std::logic_error &) {
SUCCEED();
}
}
template <typename Fn>
void ExpectDisabled(Fn &&fn) {
CaptureGuard guard;
try {
static_cast<void>(fn());
FAIL() << "Expected std::logic_error.";
} catch (const std::logic_error &) {
SUCCEED();
}
}|
|
||
| TEST(ScalarTest, SelfAssignmentPreservesValue) { | ||
| Scalar value(cytnx_double(3.5)); | ||
| value = value; |
There was a problem hiding this comment.
Direct self-assignment like value = value; can trigger compiler warnings (such as -Wself-assign or -Wself-assign-overloaded) which can break builds when -Werror is enabled. Assigning through a reference is a standard way to test self-assignment while avoiding compiler warnings.
| value = value; | |
| Scalar &ref = value; | |
| value = ref; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a780b595e7
ℹ️ 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".
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".
| void Init_by_number(const cytnx_bool &in) { | ||
| if (this->_impl != nullptr) delete this->_impl; | ||
| this->_impl = new BoolScalar(in); | ||
| this->_impl = __ScII.UScIInit[Type.cy_typeid(in)](); |
There was a problem hiding this comment.
This path is now used by every Scalar(cytnx_...) constructor, but __ScII.UScIInit is populated by the dynamically initialized __ScII object in src/backend/Scalar.cpp. If a user or another translation unit has a namespace-scope/static Scalar s(1.0), its dynamic initialization can run before __ScII, leaving these function pointers zero-initialized and calling a null pointer; the previous per-type constructors did not depend on this global factory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
True, but Tensor and UniTensor have exactly the same problem, actually much worse.
Cytnx already uses dynamically initialized global dispatch tables for Storage, Tensor storage allocation, Scalar(in, dtype), and Scalar::astype(dtype). Namespace-scope Scalar/Tensor/UniTensor objects with nontrivial initialization are therefore already unsafe. I do not think preserving direct allocation for only the one-argument Scalar constructors is worth keeping the old overload boilerplate in this PR.
❌ 16 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
Ok, so there are a bunch of The tests are not testing For example: EXPECT_EQ(ut1.at({i, j, k}) + ut2.at({i, j, k}), out.at({i, j, k}));
There is an especially clear example in // issue: for dtype = Uint64, the overload of operator- may not correct => 2 - 2 = 4.
EXPECT_EQ(ut1.at({i, j, k}) - ut2.at({i, j, k}), out.at({i, j, k}));That comment already says that there are known incorrect results! Are Cytnx developers happy to accept 2 - 2 = 4?! The test passes because it only compares Scalar a(cytnx_int64(3));
Scalar b(cytnx_double(2.0));
a /= b;
EXPECT_DOUBLE_EQ(double(a), 1.5); // old behavior gives integer result 1or, for UniTensor/Tensor arithmetic, explicitly check both the result dtype and the numeric value: UniTensor ut(..., Type.Int64);
// set an element to 3
Scalar two(cytnx_double(2.0));
ut.Div_(two);
EXPECT_EQ(ut.dtype(), Type.Double); // or expect this operation to be disabled
EXPECT_DOUBLE_EQ(ut.at<double>({0}), 1.5);I really have no suggestions for what to do at this point. The entire arithmetic type conversion system in Cytnx is irretrievably broken. If anyone cares about this, an emergency fix is to probably disable the bool/int dtypes completely, and convert integer types straight to Double. Even funnelling all integer types into Scalar a(cytnx_int64(3));
Scalar b(cytnx_double(2.0));
a /= b;results in Likewise for cytnx::Tensor a = cytnx::arange(3, 7, 1, cytnx::Type.Int64); // [3,4,5,6]
cytnx::Tensor b = cytnx::ones({4}, cytnx::Type.Double) * 2.0;
a /= b;
// Expected: [1.5, 2.0, 2.5, 3.0], dtype Double
// Actual: [1, 2, 2, 3], dtype Int64That is on CPU. The same example on GPU is memory corruption. (I didn't test that, but it is obvious from the bad pointer casts in the code). Codex suggests a reasonable emergency fix is to disable all mixed-type in-place arithmetic completely. Almost all of it is broken. The PR does this in a limited way for I am not going to work on this any further. Happy to consult with anyone who wants to work on fixes, but it is up to the people who actually use this library to determine what to do with it. |
|
We can make int a = 3;
double b = 2.0;
a /= b; // 1This is well defined in C++. a = 3
b = 2.0
a /= b # 1.5Python has a different result. |
|
The whole purpose of Scalar a(cytnx_int64(3));
Scalar b(cytnx_double(2.0));
a = a / b; // works as expected: a.dtype is Double and value is 1.5The problem is: Scalar a(cytnx_int64(3));
Scalar b(cytnx_double(2.0));
a /= b;
// Incorrect: Disabled by this PR. This used to preserve a's Int64 dtype
// and perform integer division, giving a.dtype = Int64 and value 1 |
|
A Python version. # Demonstrates that in-place division (a /= b) silently truncates and fails to
# promote dtype in cytnx, for both Scalar and Tensor, while out-of-place (a = a/b)
# is correct. Pure Python gives the correct 1.5 in both cases.
#
# Verified output (cytnx 1.1.0, CPU):
# python a=a/b : 1.5
# python a/=b : 1.5
# Scalar a=a/b : 1.5 (Double)
# Scalar a/=b : 1.0 (Int64) <-- wrong
# Tensor a=a/b : [1.5, 2.0, 2.5, 3.0] (Double)
# Tensor a/=b : [1, 2, 2, 3] (Int64) <-- wrong
#
# (The same Tensor case on GPU is expected to be memory corruption, not just a
# wrong result, due to the output-pointer cast in the in-place CUDA kernels.)
import cytnx, numpy as np
name = lambda x: cytnx.Type(x.dtype()).name
a = 3; b = 2.0; print("python a=a/b :", a / b)
a = 3; a /= b; print("python a/=b :", a)
b = cytnx.Scalar(np.float64(2.0))
a = cytnx.Scalar(np.int64(3)); a = a / b; print(f"Scalar a=a/b : {float(a)} ({name(a)})")
a = cytnx.Scalar(np.int64(3)); a /= b; print(f"Scalar a/=b : {float(a)} ({name(a)})")
b = cytnx.ones(4, dtype=cytnx.Type.Double) * 2.0
a = cytnx.arange(3, 7, 1, dtype=cytnx.Type.Int64); a = a / b; print(f"Tensor a=a/b : {a.numpy().tolist()} ({name(a)})")
a = cytnx.arange(3, 7, 1, dtype=cytnx.Type.Int64); a /= b; print(f"Tensor a/=b : {a.numpy().tolist()} ({name(a)})") |
|
Confirmed on GPU. Reproducing
Under Verified on cytnx 1.1.0, CUDA 12.9, Quadro GV100 (sm_70). Overwriting the tensor metadata end ending up with dtype == Void is genuinely odd. Memory corruption of a GPU buffer is corrupting host memory. I am confused, so is Claude and Codex, they both have no idea here. They both suggest it is some kind of secondary corruption. Further investigation proves that this is not related to overwriting the Tensor data buffer on GPU (demonstrated by patching the memory allocator to get a buffer 8x larger than requested, so no buffer overrun). It is an independent bug that happens deterministically for The } else if (tmpIN.dtype() == cytnx::Type.Bool) { ... }
} else {
cytnx_error_msg(true, "[ERROR] Void Type Tensor cannot convert to numpy ndarray%s", "\n");
}The tensor isn't Void — it's Int16, a dtype the converter doesn't handle. The catch-all message lies about the cause, and that sent Claude chasing corruption. I suspect this means that no Python users have ever created a Cytnx
|
… instead of throw Addresses review on #1011. Dispatch (@gemini-code-assist): replace the hand-written dtype->type switch (which used std::type_identity tags) in astype() with a std::visit over a value-initialized "prototype" variant materialized at the target index. The dtype<->type mapping is now the variant's own index<->alternative correspondence (already pinned by the static_asserts in Scalar.hpp), so it can't drift from Type_list. This also removes std::type_identity entirely (the project is C++20, so it was never a compatibility problem -- the switch is simply gone). In-place ops (@ianmccul): +=/-=/*=//= no longer throw on a mixed dtype. #937 disabled those paths only because the old PIMPL implementation gave wrong answers; the variant rewrite fixes that, so in-place arithmetic now follows Python value-type semantics -- `a op= b` == `a = a op b`, promoting the destination via Type.type_promote (maintainer ruling 2026-07-08). Delegates to radd/rsub/rmul/rdiv, keeping in-place and out-of-place bit-for-bit consistent; the fresh out-of-place result makes `a += a` alias-safe. binary_cmp (@gemini-code-assist): make forbid_complex a template parameter so the complex-ordering branch is discarded with `if constexpr`. Previously the runtime bool forced the compiler to type-check `op(complex, complex)` for the ordering lambdas, which only compiled by accidentally routing through operator<(Scalar, Scalar) via the implicit Scalar ctors -- a latent infinite-recursion hazard, now removed. isqrt narrows via explicit static_cast. iabs()/isqrt() stay dtype-preserving: they are unary in-place value transforms with no second operand to promote against; the abs()/iabs() consistency (#935) remains by value. Tests: convert the in-place throw pins (4 named + the 121-pair matrix in Scalar_test.cpp; 5 in scalar_variant_test.py) to assert promotion, matching out-of-place. Migrated UniTensor callers keep their out-of-place computation; stale "in-place throws" comments updated. Full C++ suite: 1606 passed, 0 failed, 11 pre-existing skips; 537 Scalar gtests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…847, fixes #935) Replace Scalar's Scalar_base* PIMPL (11 derived classes, 108 virtuals, ~2900 LOC, one heap allocation per Scalar) with a std::variant held by value, per #847's proposed layout: std::variant<std::monostate, cytnx_complex128, cytnx_complex64, cytnx_double, cytnx_float, cytnx_int64, cytnx_uint64, cytnx_int32, cytnx_uint32, cytnx_int16, cytnx_uint16, cytnx_bool> std::monostate represents the Void/uninitialized state (dtype() returns Type.Void). ScalarVariant is index-aligned with Type_list -- pinned by static_asserts in the header -- so dtype() is simply the active alternative's index. All public signatures are preserved: the 11 typed ctors, Scalar(T, dtype), explicit conversion operators, astype/conj/abs/sqrt (+ in-place forms), real()/imag(), dtype(), maxval/minval(dtype), print/ostream, all arithmetic and comparison operators, and Scalar::Sproxy. sizeof(Scalar) goes from 8 B (pointer) + a heap object to 24 B fully inline with true value semantics (copy/move/assign are `= default`, trivially correct). - self-assignment UB in operator=: eliminated by `= default`. - double-allocation copy ctor: eliminated by `= default`. - broken in-place promotion (Int64 /= Double silently doing integer division, RHS converted to destination dtype): in-place ops now throw instead of silently truncating (Ruling 1 below). - mixed-precision in-place float loss (Float += Double): now throws. - minval(Type.Double/Float) returned numeric_limits::min() (smallest positive normal); now returns ::lowest() (most negative finite). - abs()/iabs() dtype inconsistency on complex: abs() always returns the real dtype magnitude (Double/Float). iabs() keeps the complex dtype (no in-place op changes dtype) and stores the magnitude with zero imaginary part; its value now provably matches abs()'s. Documented in the header and pinned by tests. - hand-rolled enum-order promotion everywhere: every promotion decision (binary ops, comparisons, in-place guard) now routes through the single constexpr Type.type_promote() via std::visit; this also makes ComplexFloat + Double correctly yield ComplexDouble. Ruling 1 (maintainer decision 2026-07-07; BREAKING, also visible from Python where it raises RuntimeError): the in-place operators += -= *= /= throw cytnx::error when the RHS dtype does not convert into the LHS dtype, i.e. when Type.type_promote(lhs, rhs) != lhs.dtype(). Throwing classes: unsigned-LHS mixed with signed ints, int-LHS with float RHS, real-LHS with complex RHS, Bool-LHS with non-Bool. Conversions that are value-preserving per the sanctioned type_promote table (see include/Type.hpp type_promote; maintainer sign-off 2026-07-07) remain valid: same dtype; Int64 += Int32; Double += Float; ComplexDouble += Double; signed-LHS with a same-or-narrower unsigned RHS. Same-width signed/unsigned in-place mixes follow the type_promote table by explicit maintainer ruling (2026-07-07): in-place and out-of-place stay consistent, and any future tightening happens in the table. The thrown message carries only the actionable core (both dtype names, the would-be result dtype, the out-of-place/astype() remedy); the provenance lives in the guard's comment block. PR #937 (credit: @ianmccul for the guard semantics and the original tests/Scalar_test.cpp) is superseded by this commit; its test suite is absorbed and extended here. Deliberate divergences from #937's broader guard, following the ruling and the Phase-3 plan: out-of-place binary arithmetic and comparisons never throw for real dtypes (they promote via type_promote; #937 disabled mixed signed/unsigned and Bool cases outright); in-place ops whose RHS-to-LHS conversion follows the type_promote table (e.g. Int64 += Int32) are allowed because the ruling's throw categories exclude them -- #937 had disabled any differing integer dtype; and integer abs()/sqrt() remain functional (#937 disabled them). Ordering comparisons on complex still throw; complex equality is allowed (as before). A known consequence of the table -- Scalar(Int64(-1)) == Scalar(Uint64 max) evaluating true via the Int64 promotion -- is pinned by test with maintainer sign-off. Storage_base::set_item(idx, const Scalar&) is re-implemented via std::visit on the Scalar's active alternative, forwarding the concrete value into the existing typed SetItem<T> (monostate throws instead of writing garbage). get_item already constructed the Scalar from the typed at<T>() and is unchanged. Migrated in-tree callers that relied on lossy in-place mixes (none were covered by existing tests; test tally is baseline + new, nothing removed): - BlockUniTensor::normalize_() and BlockFermionicUniTensor:: normalize_(): the norm^2 accumulator is now seeded with the floating dtype linalg::Norm() actually produces instead of this->dtype(), which for an integer-dtype UniTensor was an Int += Double lossy mix. The Norm output-dtype policy is centralized in a new constexpr Type_class::norm_result_dtype() (include/Type.hpp), used by both normalize_ sites and by src/linalg/Norm.cpp itself (replacing its hand-rolled enum-order dtype checks). - DenseUniTensor::Add_/Sub_ (dense += diagonal element loop): switched to out-of-place `v = v +/- rhs` since the two UniTensors' dtypes may legitimately differ (per #937's own migration guidance). - linalg_internal_cpu/{Add,Sub,Mul,Cpr}_internal.hpp: replaced the incidental Scalar_init_interface::type_promote_t alias (inherited from Type_class by the deleted PIMPL factory class) with Type_class::type_promote_t directly. - linalg_internal_cpu/Gemm_Batch_internal.hpp: replaced direct x._impl->to_cytnx_complex128()/64() with the public complex128()/ complex64() accessors. Tests: absorbs tests/Scalar_test.cpp (based on #937's, adjusted to the ruling's matrix) and extends it with a programmatic 11x11 dtype matrix over + - * / and comparisons (asserting result dtype == Type.type_promote(a,b) and value correctness), an 11x11 in-place throw matrix (throws iff type_promote(lhs,rhs) != lhs), and a pinned mixed-signedness equality quirk test. Adds pytests/scalar_variant_test.py covering Python arithmetic across dtypes, the new in-place RuntimeError behavior, Storage indexed assignment from Scalar, and numpy-scalar constructor paths (pinning a pre-existing pybind quirk where non-int64 integer numpy scalars resolve to the Int64 ctor overload). pybind/scalar_py.cpp is unchanged: none of its 84 bindings touched Scalar internals. Gates: C++ 1617 ran / 1602 passed (1086 baseline + 516 new) / 11 skipped / same 4 known linalg_Test.BkUt_* failures (#977). pytest 100 passed (57 baseline + 43 new). grep Scalar_base in include/ src/ pybind/ -> zero hits. build_py warning-clean. BREAKING CHANGE: mixed-dtype in-place Scalar arithmetic that would change or truncate the LHS dtype now throws cytnx::error (Python: RuntimeError) instead of silently truncating. Use out-of-place arithmetic or astype(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… instead of throw Addresses review on #1011. Dispatch (@gemini-code-assist): replace the hand-written dtype->type switch (which used std::type_identity tags) in astype() with a std::visit over a value-initialized "prototype" variant materialized at the target index. The dtype<->type mapping is now the variant's own index<->alternative correspondence (already pinned by the static_asserts in Scalar.hpp), so it can't drift from Type_list. This also removes std::type_identity entirely (the project is C++20, so it was never a compatibility problem -- the switch is simply gone). In-place ops (@ianmccul): +=/-=/*=//= no longer throw on a mixed dtype. #937 disabled those paths only because the old PIMPL implementation gave wrong answers; the variant rewrite fixes that, so in-place arithmetic now follows Python value-type semantics -- `a op= b` == `a = a op b`, promoting the destination via Type.type_promote (maintainer ruling 2026-07-08). Delegates to radd/rsub/rmul/rdiv, keeping in-place and out-of-place bit-for-bit consistent; the fresh out-of-place result makes `a += a` alias-safe. binary_cmp (@gemini-code-assist): make forbid_complex a template parameter so the complex-ordering branch is discarded with `if constexpr`. Previously the runtime bool forced the compiler to type-check `op(complex, complex)` for the ordering lambdas, which only compiled by accidentally routing through operator<(Scalar, Scalar) via the implicit Scalar ctors -- a latent infinite-recursion hazard, now removed. isqrt narrows via explicit static_cast. iabs()/isqrt() stay dtype-preserving: they are unary in-place value transforms with no second operand to promote against; the abs()/iabs() consistency (#935) remains by value. Tests: convert the in-place throw pins (4 named + the 121-pair matrix in Scalar_test.cpp; 5 in scalar_variant_test.py) to assert promotion, matching out-of-place. Migrated UniTensor callers keep their out-of-place computation; stale "in-place throws" comments updated. Full C++ suite: 1606 passed, 0 failed, 11 pre-existing skips; 537 Scalar gtests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
This PR disables, via throwing
std::logic_error, a few (but definitely not all) of theScalararithmetic operations that are capable of returning incorrect results. I have no idea whether any downstream code uses these operations, but since they are exposed via Python, it seems that leaving them in the library as they are is dangerous. At least this PR will make the code give a sensible error message rather than silently producing nonsense. The disabled arithmetic is mainly mixed signed/unsigned arithmetic and mixed integer/floating-point. Probably no one ever uses this anyway.If any code is affected by this, the fix is to replace in-place arithmetic by out-of-place arithmetic. Eg
This was uncovered by a cursory examination of
Scalar.hppsource code in the aftermath of #935. But this PR does not in any way resolve #935.This PR also adds
tests/Scalar_tests.cpp, since there were no tests ofScalarbefore. It tests that some simple cases of Scalar arithmetic work as expected, and also verifies that some broken arithmetic combinations are in fact disabled. I expect that there remain some broken combinations not fixed by this PR.As some drive-by cleanups, this PR removes a few of the unnecessary overloaded functions and replaces them with constrained templates, using
concept CytnxType. This is a pattern that could be used elsewhere:Unfortunately it is not possible to remove the vast majority of pointless overloads, because they are
virtualand in C++ templates cannot be virtual.Also discovered in the course of writing this PR message:
This actually works: it uses a completely different code path to
Scalar:src/Tensor.cpp:799, which callslinalg::Div(*this, rc)and replaces the storage.Div<Tensor, cytnx_double>allocates output storage withType.type_promote(Type.Double, Lt.dtype()), so the result dtype becomes Double.But if the RHS is a
Tensorwhosedtypeis Double,then on CPU, it goes through
linalg::iDivatsrc/Tensor.cpp:779. On CPU that dispatches true in-place kernels, writing back into the original integer storage. So 3 / 2.0 becomes integer-truncated 1, andt.dtype()stays integer.On GPU the behavior is quite different:
The GPU goes through the old
cuAri_iipath insrc/linalg/iDiv.cpp:34. For Int64 / Double, it createstmpo = Lt.clone(), sotmpois still Int64, but dispatchescuDiv_internal_i64td, whose output pointer is cast tocytnx_double*insrc/backend/linalg_internal_gpu/cuDiv_internal.cu:2096.The result is memory corruption since the
double*data is written into a buffer withdtypeInt64.For integers of size smaller than the right hand side, it is also memory-unsafe and will overwrite the buffer.
I haven't submitted an issue on this. Just another drop in the ocean...