Skip to content

Add a reference constructor to Span. Replace VectorView with Span #106389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented May 14, 2025

This PR adds an implicit conversion from any value reference to a single-element Span, matching a constructor VectorView already had.
With this addition, VectorView can finally be merged into Span.

Span from reference

I finally found a way to satisfyingly support single reference constructors for Span!

My initial implementation was the same as VectorView, implemented in #103931. However, I was not happy with it because it meant that Span could be implicitly created from any value, which made me very uneasy.

Still, after having thought about it for a while, it does make sense for a value to be passed into a function that expects a Span: It just means we want to run the function on the one value. For example, in the future the function could be overloaded with a single value version, automatically optimizing all callers that were previously implicitly converting to Span.

An insight came in the form of implicit type checks. Consider the following code example:

template <typename T>
class Span {
	const T *_ptr = nullptr;
	uint64_t _len = 0;

public:
	 constexpr Span(const T &p_value) :
			_ptr(&p_value), _len(1) {}
};


int main() {
	int i = 0;
	Span<int> span1(i);
	Span<double> span2(i);
	return 0;
}

This compiles fine, but will crash horribly if span2 is ever used, because i is implicitly converted to double, but the temporary is dropped after the call, making the Span immediately garbage.
However, using the following implementation (used in this PR) makes this much safer:

    template <typename T1, typename = std::enable_if_t<std::is_same_v<T1, T>>>
	 constexpr Span(const T1 &p_value) :
			_ptr(&p_value), _len(1) {}

Using this constructor instead, Span<double> span2(i) will statically fail because implicit conversions are not allowed for this constructor.

VectorView to Span

VectorView was initially intended to be used widely across the codebase:

// This may one day be used in Godot for interoperability between C arrays, Vector and LocalVector.
// (See https://github.com/godotengine/godot-proposals/issues/5144.)

Span is this implementation. With the reference constructor out of the way, the two can finally be merged. All that was required was a search-and-replace from VectorView to Span.

Notably, the use of SFINAE requires explicit conversions:

void test(Span<int> span) {
    // ...
}

void main() {
    test(5); // fine
    test(5.0); // impossible
}

In this case it would probably be better to just accept implicit conversions. But I do feel safer with these checks, and we can always generalize if this becomes a problem.

Dangers

Converting from a reference to Span can be considered dangerous. Consider the following code:

Span<int> get_span() {
	int i = 0;
	return i;
}

This will obviously go horribly wrong.

However, this problem already exists in C++ plain:

int& get_int() {
	int i = 0;
	return i;
}

This compiles fine, but will also go horribly wrong.
The main difference is that the compiler will (usually) warn about the latter implementation, but may not understand Span well enough to also warn about the former implementation.

All in all, I think this risk is acceptable, considering C++ is full of memory pitfalls, and this isn't any worse than what C++ has in store for us in other places.

@Ivorforce Ivorforce added this to the 4.5 milestone May 14, 2025
@Ivorforce Ivorforce requested a review from clayjohn May 14, 2025 09:09
@Ivorforce Ivorforce requested review from a team as code owners May 14, 2025 09:09
@Ivorforce Ivorforce force-pushed the vector-view-more-like-span branch from 8828662 to af9bed0 Compare May 14, 2025 09:15
@Ivorforce Ivorforce requested a review from a team as a code owner May 14, 2025 09:15
@Ivorforce Ivorforce force-pushed the vector-view-more-like-span branch from af9bed0 to 500eb36 Compare May 14, 2025 09:16
…ccidental conversions.

Remove `VectorView`, in favour of `Span`.
@Ivorforce Ivorforce force-pushed the vector-view-more-like-span branch from 500eb36 to 45680f9 Compare May 14, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant