Make invalid types passed to cv_typeid_v<T> a compile-time error#938
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 introduces variant_contains_v and adds a compile-time check for variant_index when a type is not found in a variant, along with corresponding unit tests. The reviewer suggested simplifying the implementation of variant_contains_v by using a partially specialized variable template instead of a helper struct to reduce boilerplate.
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 <typename T, typename Variant> | ||
| struct variant_index; | ||
| struct variant_contains; | ||
|
|
||
| template <typename T, typename... Types> | ||
| struct variant_index<T, std::variant<Types...>> { | ||
| static constexpr std::size_t value = std::variant_size_v<std::variant<Types...>>; | ||
| struct variant_contains<T, std::variant<Types...>> | ||
| : std::bool_constant<(std::is_same_v<T, Types> || ...)> {}; | ||
|
|
||
| template <typename T, typename Variant> | ||
| inline constexpr bool variant_contains_v = variant_contains<T, Variant>::value; |
There was a problem hiding this comment.
We can simplify the implementation of variant_contains_v by defining it directly as a partially specialized variable template. This eliminates the need for the helper struct variant_contains entirely, reducing boilerplate and template instantiation overhead.
| template <typename T, typename Variant> | |
| struct variant_index; | |
| struct variant_contains; | |
| template <typename T, typename... Types> | |
| struct variant_index<T, std::variant<Types...>> { | |
| static constexpr std::size_t value = std::variant_size_v<std::variant<Types...>>; | |
| struct variant_contains<T, std::variant<Types...>> | |
| : std::bool_constant<(std::is_same_v<T, Types> || ...)> {}; | |
| template <typename T, typename Variant> | |
| inline constexpr bool variant_contains_v = variant_contains<T, Variant>::value; | |
| template <typename T, typename Variant> | |
| inline constexpr bool variant_contains_v = false; | |
| template <typename T, typename... Types> | |
| inline constexpr bool variant_contains_v<T, std::variant<Types...>> = | |
| (std::is_same_v<T, Types> || ...); |
There was a problem hiding this comment.
Yes, that is a nice improvement.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #938 +/- ##
=======================================
Coverage 30.87% 30.87%
=======================================
Files 229 229
Lines 34757 34757
Branches 14409 14409
=======================================
Hits 10730 10730
Misses 16720 16720
Partials 7307 7307
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Reviewed the change. This is a clean, well-scoped, correct fix. Replacing the silent "return one-past-the-end" fallback with a static_assert is the right call, and the template metaprogramming is sound.
Correctness — verified, no issues:
- The recursion terminates correctly: a missing type strips the variant down to
std::variant<>, which hits thestatic_assert.always_false_v<T>is the correct dependent-false idiom (won't fire prematurely), and no partial-specialization ambiguity is introduced. - Audited all callers of
variant_index_v/cy_typeid_v/cy_typeid_gpu_v/cy_typeid. Every instantiation uses a type guaranteed to be in the relevant list, so nothing that previously compiled now breaks:Tensor.hpp:522(ptr_as) → hostType_list;Tensor.hpp:545(gpu_ptr_as) correctly uses the separatecy_typeid_gpu_voverType_list_gpu.Add.cpp:20(AddScalarTensorImpl<TL>) is protected — the genericAdd<T>hasstatic_assert(!is_same_v<T, Scalar>)andAdd<Scalar>is specialized separately, socy_typeid_v<Scalar>is never instantiated.- GPU dispatch files consistently use
cy_typeid_gpu_v<TO>over GPU-list types.
Minor, non-blocking:
tests/Type_test.cpp— the PR's stated subjectcy_typeid_visn't directly tested; the tests exercisevariant_index_v/variant_contains_v. Astatic_assert(cy_typeid_v<cytnx_double> == 3)would lock the user-facing API. (The negative/compile-error case is necessarily untestable in gtest — fine.)- The runtime
TEST(TypeTest, VariantContainsRecognizesSupportedTypes)duplicates the threestatic_assert(variant_contains_v<...>)lines directly above it, so its body adds no coverage beyond serving as a gtest anchor.
LGTM — the two notes are optional test polish, not blockers.
Posted by Claude Code on behalf of @pektiong
|
Format check is not passed. |
I think that is a difference between clang-format-14 and newer clang-format. |
Yes, different clang-format has different default formats. |
|
@IvanaGyro are we using clang-format-14 or newer clang-format? |
We use version 14. Here is the pre-commit setting: Lines 9 to 13 in 9b6a407 |
The Formatting Check CI runs clang-format 14 (pinned in .pre-commit-config.yaml as v14.0.6). Reflow the variant_contains_v fold expression to match; no functional change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 868d1165e0
ℹ️ 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".
|
|
||
| template <typename T> | ||
| struct variant_index<T, std::variant<>> { | ||
| static_assert(always_false_v<T>, "variant_index<T, Variant>: T is not in Variant"); |
There was a problem hiding this comment.
In CUDA builds (UNI_GPU), the existing GPU Kron dispatch instantiates out.ptr_as<out_type>() in src/linalg/Kron.cpp:90 for every Type_list_gpu alternative; when either input is complex, out_type becomes cuDoubleComplex or cuComplex, which are intentionally not in the CPU Type_list. This new base-case assertion therefore fires while compiling Type_class::cy_typeid_v<out_type> inside ptr_as, so the CUDA build breaks for complex Kron unless that path is updated to use the GPU dtype mapping before making missing indices hard errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6e1c1ab1. src/linalg/Kron.cpp:90 (the GPU dispatch) now calls out.gpu_ptr_as<out_type>() instead of out.ptr_as<out_type>(), so the lookup for a Type_list_gpu out_type (cuDoubleComplex/cuComplex) goes through cy_typeid_gpu_v/Type_list_gpu instead of the CPU Type_list — avoiding the new hard variant_index error and keeping the UNI_GPU complex-Kron build compiling. The CPU call at line 75 stays ptr_as (its out_type is a CPU Type_list member).
Caveat: CI has no CUDA build, so this is correct-by-inspection (and by Type_list/Type_list_gpu index alignment) rather than compile-verified here — a real UNI_GPU build is the confirming check.
Posted by Claude Code on behalf of @pcchen
# Conflicts: # tests/CMakeLists.txt
The GPU Kron path derives out_type from Type_promote_from_gpu_pointer_t (a Type_list_gpu member, e.g. cuDoubleComplex/cuComplex for complex inputs), but called out.ptr_as<out_type>(), which maps through the CPU Type_list via cy_typeid_v. With variant_index now a hard compile-time error for types not in the list (this PR), that instantiation breaks the UNI_GPU build for complex Kron. Use gpu_ptr_as<out_type>() so the lookup goes through Type_list_gpu, as the ptr_as/gpu_ptr_as contract intends. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed two commits to 1. Merged current 2. Fixed the P1 CUDA-dispatch issue ( Note: CI has no CUDA build, so the Posted by Claude Code on behalf of @pcchen |
|
Summary
Testing
|
|
Interesting. Kron on GPU would fail at runtime for the last 2 years - almost a perfect demonstration of Using the CUDA ABI types is consistent with the old Kron kernel back from 128071a but its not really needed. Better to deprecate |
3 participants
Short PR to make
cy_typeid_v<T>a compile error ifTis an invalid type.cy_typeid_v<T>is a compile-time integer that returns thedtypecorresponding to the typeT. Due to an oversight, ifTis not one of the 12-supported types withdtype = 0 .. 11,cy_typeid_v<T>would get the value 12. If this is used as an index into an array, it will likely result in reading one-past-the-end.Root cause:
This was an oversight when the
cy_typeid_v<T>mechanism was added. There is avariant_index<T, Type_list>mechanism added which returns the index of a type in astd::variant. If the type does not exist in the supplied variant, the previous behavior was to return the size of the variant (i.e. one-past-the-end of a valid index). This is now a compile-timestatic_assert: "variant_index<T, Variant>: T is not in Variant".In keeping with common C++ conventions,
cy_typeid_v<T>expects an exact match for the typeT. Socy_typeid_v<const double>is a compile-time error, as is other cv or reference-qualified types. This is a good fix because previous behavior would result incy_typeid_v<const double> == 12.If
Tis not expected to be an exact type, then normalize the type with:cy_typeid_v<std::remove_cvref_t<T>>