close
Skip to content

Enable range based looping of sf::VertexArray#3366

Merged
ChrisThrasher merged 1 commit into
masterfrom
vertex_array_begin_end
Apr 1, 2025
Merged

Enable range based looping of sf::VertexArray#3366
ChrisThrasher merged 1 commit into
masterfrom
vertex_array_begin_end

Conversation

@ChrisThrasher

@ChrisThrasher ChrisThrasher commented Dec 26, 2024

Copy link
Copy Markdown
Member

Description

begin and end iterators are all that is required of a container to supported C++11 ranged-for loops. This PR also uses this feature as much as possible internally. Let me know if I missed any more places we can be using this.

@ChrisThrasher ChrisThrasher added this to the 3.1 milestone Dec 26, 2024
@coveralls

coveralls commented Dec 26, 2024

Copy link
Copy Markdown
Collaborator

Pull Request Test Coverage Report for Build 12536923590

Details

  • 25 of 27 (92.59%) changed or added relevant lines in 3 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.004%) to 57.02%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/SFML/Graphics/Text.cpp 2 4 50.0%
Files with Coverage Reduction New Missed Lines %
src/SFML/Window/WindowImpl.cpp 4 52.29%
Totals Coverage Status
Change from base Build 12509336161: -0.004%
Covered Lines: 12068
Relevant Lines: 20002

💛 - Coveralls

@kimci86

kimci86 commented Dec 27, 2024

Copy link
Copy Markdown
Contributor

VertexArray::operator[] is not covered by tests now.

@ChrisThrasher

Copy link
Copy Markdown
Member Author

VertexArray::operator[] is not covered by tests now.

This operator is still used indirectly by some other tests.

@kimci86

kimci86 commented Dec 27, 2024

Copy link
Copy Markdown
Contributor

This operator is still used indirectly by some other tests.

Which one? Coveralls report does not see it.

@ChrisThrasher

ChrisThrasher commented Dec 27, 2024

Copy link
Copy Markdown
Member Author

This operator is still used indirectly by some other tests.

Which one? Coveralls report does not see it.

CHECK(vertexArray[0].position == vertex.position);

It’s used in a few places. It’s weird that coverage doesn’t catch this.

@kimci86

kimci86 commented Dec 28, 2024

Copy link
Copy Markdown
Contributor

Ok, this is because the non-const variant is used by tests but the const variant is not.

@ChrisThrasher

Copy link
Copy Markdown
Member Author

Ok, this is because the non-const variant is used by tests but the const variant is not.

Ah okay this makes sense. I added tests explicitly for this case to make sure it remains covered. Thanks for catching this!

Comment thread test/Graphics/VertexArray.test.cpp Outdated
@ChrisThrasher ChrisThrasher force-pushed the vertex_array_begin_end branch from a25a87e to 508c3ea Compare March 29, 2025 19:02
@eXpl0it3r

Copy link
Copy Markdown
Member

Am I understanding it correctly, that this opens up additional opens to manipulate the VertexArray's vertices? Anything that takes a begin()-end() "range", right? Is there anything we need to consider in those cases?

What I mean, the PR focuses on ranged-based looping, but the feature itself goes beyond that. So I just want to make sure when people do manipulate the vertices through that interface, nothing breaks. I currently don't see anything, but maybe someone else can think of some potential problem.

@ZXShady

ZXShady commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Am I understanding it correctly, that this opens up additional opens to manipulate the VertexArray's vertices? Anything that takes a begin()-end() "range", right? Is there anything we need to consider in those cases?

What I mean, the PR focuses on ranged-based looping, but the feature itself goes beyond that. So I just want to make sure when people do manipulate the vertices through that interface, nothing breaks. I currently don't see anything, but maybe someone else can think of some potential problem.

they can't break anything

begin and end are just equalivent semanticly to &vertex_array[0] and &vertex_array[vertex_array.size()] respectivly but less annoying to type.

@ChrisThrasher

Copy link
Copy Markdown
Member Author

As far as I’m aware, this is purely syntactic sugar. It doesn’t enable any functionality that wasn’t previously possible through an existing interface.

@eXpl0it3r eXpl0it3r left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a useful addition 👍

@ChrisThrasher ChrisThrasher merged commit 7a6b7f4 into master Apr 1, 2025
@ChrisThrasher ChrisThrasher deleted the vertex_array_begin_end branch April 1, 2025 15:49
@github-project-automation github-project-automation Bot moved this from In Review to Done in SFML 3.1.0 Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants