Skip to content
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

Implement Range Iterators #50284

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

reduz
Copy link
Member

@reduz reduz commented Jul 8, 2021

This PR implements range iterators in the base containers (Vector, Map, List, Pair Set).
Given several of these data structures will be replaced by more efficient versions, having a common iterator API will make this simpler.
Iterating can now be done as follows (examples):

//Vector<String>
for(const String& I: vector) {

}
//List<String>
for(const String& I: list) {

}
//Map<String,int>
for(const KeyValue<String,int>&I : map) {
	print_line("key: "+I.key+" value: "+itos(I.value));

}

//if intending to write the elements, reference can be used

//Map<String,int>
for(KeyValue<String,int>& I: map) {
	I.value = 25;
	//this will fail because key is always const
	//I.key = "hello"
}

The containers are (for now) not STL compatible, since this would mean changing how they work internally (STL uses a special head/tail allocation for end(), while Godot Map/Set/List do not).
The idea is to change the Godot versions to be more compatible with STL, but this will happen after conversion to new iterators have taken place.

Note that for better code readability (specially when we are reviewing PRs), auto is still banned. Iterators must be defined with the right type.

After this Pull Request is merged, we will be looking for contributors to lend a hand converting the codebase to use this new syntax as much as possible.

Some rules for the conversion work:

  • always use const reference for the iterator value, unless intending to write to the value, in which case use a regular reference.
  • If Set/List/Map container iteration is done in reverse, or does something strange, leave as is, will convert when we can reimplement end() to be a proper tail object (after everything else was converted), since this requires the next() at the end of the list to return the tail. This will allow for going in reverse or implement a reversible adapter more easily.

@reduz reduz requested a review from a team as a code owner July 8, 2021 15:42
@Calinou Calinou added this to the 4.0 milestone Jul 8, 2021
@Calinou
Copy link
Member

Calinou commented Jul 8, 2021

Should this also be implemented in LocalVector, Array and Dictionary?

core/templates/list.h Outdated Show resolved Hide resolved
@reduz
Copy link
Member Author

reduz commented Jul 8, 2021

@Calinou possibly, but I would wait since those need to be converted different.

@reduz reduz force-pushed the implement-range-iterators branch from 829db94 to 03f7aa5 Compare July 8, 2021 15:56
core/templates/vector.h Outdated Show resolved Hide resolved
core/templates/pair.h Outdated Show resolved Hide resolved
core/templates/vector.h Outdated Show resolved Hide resolved
editor/audio_stream_preview.h Show resolved Hide resolved
@reduz reduz force-pushed the implement-range-iterators branch from 03f7aa5 to e949ee7 Compare July 8, 2021 20:49
core/templates/list.h Outdated Show resolved Hide resolved
@reduz reduz force-pushed the implement-range-iterators branch from e949ee7 to efd92e8 Compare July 9, 2021 02:26
This PR implements range iterators in the base containers (Vector, Map, List, Pair Set).
Given several of these data structures will be replaced by more efficient versions, having a common iterator API will make this simpler.
Iterating can be done as follows (examples):

```C++
//Vector<String>
for(const String& I: vector) {

}
//List<String>
for(const String& I: list) {

}
//Map<String,int>
for(const KeyValue<String,int>&I : map) {
	print_line("key: "+I.key+" value: "+itos(I.value));

}

//if intending to write the elements, reference can be used

//Map<String,int>
for(KeyValue<String,int>& I: map) {
	I.value = 25;
	//this will fail because key is always const
	//I.key = "hello"
}

```

The containers are (for now) not STL compatible, since this would mean changing how they work internally (STL uses a special head/tail allocation for end(), while Godot Map/Set/List do not).
The idea is to change the Godot versions to be more compatible with STL, but this will happen after conversion to new iterators have taken place.
@reduz reduz force-pushed the implement-range-iterators branch from efd92e8 to a9c943b Compare July 9, 2021 02:27
@akien-mga akien-mga merged commit 96e17e4 into godotengine:master Jul 9, 2021
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Jul 13, 2021

With:

// Also fails with:
// const Vector<Vector3> vertices = arrays[Mesh::ARRAY_VERTEX];
const PackedVector3Array vertices = arrays[Mesh::ARRAY_VERTEX];

for (const Vector3& vertex : vertices) { // Same error with Vector3 (without `&`).
	st->add_vertex(vertex);
}

I get the following compilation error with Clang 12:

Compiling ==> scene/3d/occluder_instance_3d.cpp
In file included from scene/3d/occluder_instance_3d.cpp:31:
In file included from scene/3d/occluder_instance_3d.h:34:
In file included from ./scene/3d/visual_instance_3d.h:34:
In file included from ./core/math/face3.h:34:
In file included from ./core/math/aabb.h:35:
In file included from ./core/math/plane.h:34:
In file included from ./core/math/vector3.h:35:
In file included from ./core/math/vector3i.h:34:
In file included from ./core/string/ustring.h:36:
./core/templates/vector.h:248:10: error: no matching conversion for functional-style cast from 'const Vector3 *' to 'Vector<Vector3>::ConstIterator'
                return ConstIterator(ptr());
                       ^~~~~~~~~~~~~~~~~~~
scene/3d/occluder_instance_3d.cpp:280:33: note: in instantiation of member function 'Vector<Vector3>::begin' requested here
                                        for (const Vector3& vertex : vertices) {
                                                                   ^
./core/templates/vector.h:232:3: note: candidate constructor not viable: 1st argument ('const Vector3 *') would lose const qualifier
                ConstIterator(T *p_ptr) { elem_ptr = p_ptr; }
                ^
./core/templates/vector.h:234:3: note: candidate constructor not viable: no known conversion from 'const Vector3 *' to 'const Vector<Vector3>::ConstIterator' for 1st argument
                ConstIterator(const ConstIterator &p_it) { elem_ptr = p_it.elem_ptr; }
                ^
./core/templates/vector.h:233:3: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
                ConstIterator() {}
                ^
./core/templates/vector.h:251:10: error: no matching conversion for functional-style cast from 'const Vector3 *' to 'Vector<Vector3>::ConstIterator'
                return ConstIterator(ptr() + size());
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
scene/3d/occluder_instance_3d.cpp:280:33: note: in instantiation of member function 'Vector<Vector3>::end' requested here
                                        for (const Vector3& vertex : vertices) {
                                                                   ^
./core/templates/vector.h:232:3: note: candidate constructor not viable: 1st argument ('const Vector3 *') would lose const qualifier
                ConstIterator(T *p_ptr) { elem_ptr = p_ptr; }
                ^
./core/templates/vector.h:234:3: note: candidate constructor not viable: no known conversion from 'const Vector3 *' to 'const Vector<Vector3>::ConstIterator' for 1st argument
                ConstIterator(const ConstIterator &p_it) { elem_ptr = p_it.elem_ptr; }
                ^
./core/templates/vector.h:233:3: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
                ConstIterator() {}
                ^
2 errors generated.

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.

5 participants