Add implicit constructor to Span#2348
Draft
sethrj wants to merge 29 commits intoceleritas-project:developfrom
Draft
Add implicit constructor to Span#2348sethrj wants to merge 29 commits intoceleritas-project:developfrom
sethrj wants to merge 29 commits intoceleritas-project:developfrom
Conversation
sethrj
added
enhancement
There was a problem hiding this comment.
Pull request overview
This PR updates corecel::Span to support an implicit, constraints-checked converting constructor (notably enabling standard-like const conversion while preventing LdgSpan from implicitly degrading to Span), and adjusts related utilities/tests/docs to match.
Changes:
- Add a constrained converting
Span(Span<U, N> const&)constructor using type- and extent-compatibility checks. - Remove a now-redundant
load_array(Span<T, N>)overload that previously existed to work around missing implicit const conversion. - Update unit tests and documentation to reflect the new conversion behavior and reorganized API docs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/corecel/cont/Span.test.cc | Switch to stream_to_string and add compile-time/runtime tests for new conversion constraints. |
| src/corecel/math/detail/SpanUtilsImpl.hh | Remove mutable-span load_array overload now that const conversion is supported. |
| src/corecel/data/LdgIterator.hh | Documentation clarifications for LdgValue/to_array behavior. |
| src/corecel/data/Collection.hh | Minor documentation fixes/cleanup. |
| src/corecel/cont/Span.hh | Introduce constrained converting constructor; update Span documentation and some size_t-typed APIs. |
| src/corecel/cont/detail/SpanImpl.hh | Add is_span_size_convertible and is_array_convertible_v helpers; minor constant cleanup. |
| doc/implementation/data-model.rst | Reorganize data-model documentation sections and references. |
| doc/implementation/corecel/stdlib.rst | Add/relocate “Containers” section and doxygen entries for container types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test summary 260 files 421 suites 7s ⏱️ Results for commit 1589398. ♻️ This comment has been updated with latest results. |
Replace the LdgTraits specialization-based customization point with an ADL-based free function ldg_data(T const*) -> U const*. The built-in overloads for arithmetic and enum types live in LdgTraits.hh; the OpaqueId and Quantity overloads now live alongside their respective types in OpaqueId.hh and Quantity.hh. The is_ldg_supported_v trait is re-expressed using void_t SFINAE detection over the ADL call. Update ldg() in LdgRefWrapper.hh to call ldg_data via ADL and derive the arithmetic check from the returned pointer type rather than a traits member. Add a two-argument ldg(obj, mp) convenience overload and a storable LdgMember<Class,T> functor with a CTAD deduction guide for loading a member-pointer field from a struct via __ldg. Add test coverage for both the two-argument ldg and LdgMember callable. Prompt: "Please implement those changes in the code, replacing all use of the old LdgTraits." Assisted-by: GitHub Copilot
Replace the one-line \page stub with a complete reference page that covers: - What __ldg does and when to use it (scalar, struct member, span) - Code examples for each usage pattern including LdgMember and LdgSpan - How to extend ldg support to a custom type via ldg_data ADL overload - Built-in overloads for arithmetic, enum, OpaqueId, and Quantity - Brief descriptions of LdgWrapper and LdgIterator implementation detail Also cleans up the broken placeholder text in the ldg() function doc comment. Prompt: "Combine the main documentation of the ldg-related functions into a doxygen `\page ldg Cached device loading`. Provide examples of usage in user code (data, struct, span), and of overloading the ldg_data. Briefly mention and summarize the under-the-hood LdgWrapper and LdgIterator." Assisted-by: GitHub Copilot
This reverts commit 38edd6e.
Add SFINAE to the `Span<T, Extent>(Span<U, N> const&)` constructor so it only participates in overload resolution when the conversion is safe: - `std::is_convertible_v<U(*)[], element_type(*)[]>` — the standard array-pointer trick that allows const-qualification (T → T const) but blocks const removal (T const → T) and unsafe covariant conversions - `N == dynamic_extent || Extent == dynamic_extent || N == Extent` — extents must be compatible (same fixed size, or either side dynamic) This mirrors the C++20 std::span converting constructor requirements. Test static_asserts confirm const→mutable and mismatched fixed-extent constructions are rejected at compile time. Prompt: "Update the span class to safely and implicitly construct from a span of a convertible pointer type (e.g. from T to const T)." Assisted-by: GitHub Copilot (claude-sonnet-4-5)
Add operator==(LdgWrapper, LdgWrapper) and operator!=(LdgWrapper, LdgWrapper) overloads to LdgWrapper to resolve ambiguity when std::equal iterates over two LdgSpan ranges. Previously, comparing two LdgWrapper instances matched both operator==(LdgWrapper, type) and operator==(type, LdgWrapper) after implicit conversion, causing a hard compile error via libc++ __equal_to. Add a LdgWrapperTest.equality test that exercises LdgWrapper == LdgWrapper comparisons directly and via std::equal over two LdgSpans. Prompt: "Fix the build errors due to LdgWrapper's ambiguous types. Add new tests for LdgWrapper equality that demonstrates the build failure and its fix." Assisted-by: GitHub Copilot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the proximate cause of the LDG refactoring (#2343) which led to the friend stuff (#2347). Adding the implicit cast with conversion checking is necessary to be more standard-compliant, and to allow us to integrate the span utils for vecgeom usage, but it prevents
LdgSpanfrom being implicitly cast toSpan. This is good because we want to not accidentally loseldgacceleration, which we're currently doing in a couple of places.