Use range-based for loops in dialup and mediaplayer samples (PR #26640)
Nong Thi Quynh Anh
This PR modernizes loops in the samples/ directory by replacing index‑based iteration with C++11 range‑based for loops where appropriate.
- These changes improve readability and reduce the risk of indexing errors.
- Only loops that do not rely on the index for logic were modified.
- No behavior changes are introduced.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26640
Commit Summary
- b063caa Use range-based for loops in dialup and mediaplayer samples
File Changes
(2 files)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26640.patch
- https://github.com/wxWidgets/wxWidgets/pull/26640.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Nong Thi Quynh Anh
anhnong90 left a comment (wxWidgets/wxWidgets#26640)Hi, just a small note:
The Ubuntu 18.04 GTK2/GTK3 jobs seem to fail due to the CI environment itself. The logs show a PPA/GPG issue during setup, long before the build reaches any of the sample code. All other platforms passed fine. This PR only updates simple loops in the samples and doesn’t change behavior.
Thanks a lot for taking a look!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26640)Thanks, this can be merged but are these the only example of for loops that could be modernized? I'm a bit surprised by this... Have you done this manually or did you try running clang-tidy which I think has a check to do this automatically?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Nong Thi Quynh Anh
anhnong90 left a comment (wxWidgets/wxWidgets#26640)Thanks for the feedback!
I found these loops while working through the samples/, and decided to modernize them to make the sample code a bit clearer for newer learners. I searched the samples/ folder using some useful commands like:
git grep -n "for (size_t" samples/
git grep -n "for.*size_t.*<.*Count"
and manually checked each match to ensure it was safe to convert. These were the only cases that met the criteria.
I haven’t used clang‑tidy for this pass, but I’m happy to do so — and I’m also willing to continue in this direction in more areas if that would be helpful.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Nong Thi Quynh Anh
@anhnong90 pushed 2 commits.
- 2e09f4f Merge branch 'master' into modernize-sample-loops
- 187af2e Modernize loops across core source files using clang-tidy
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Sorry, I stopped reviewing after the first few files because while the changes are good, per se, we really should use loop variable names that make sense. Could you please recheck them? TIA!
In include/wx/private/fswatcher.h:
> @@ -87,11 +87,9 @@ class wxFSWatcherImpl
virtual bool RemoveAll()
{
bool ret = true;
- for ( wxFSWatchEntries::iterator it = m_watches.begin();
- it != m_watches.end();
- ++it )
+ for (auto & m_watche : m_watches)
Not sure if this is clang-tidy doing, but this variable name is bad, it should be watch, of course.
It should also probably be const...
> @@ -80,12 +80,12 @@ class wxPipe
// close the pipe descriptors
void Close()
{
- for ( size_t n = 0; n < WXSIZEOF(m_fds); n++ )
+ for (int & m_fd : m_fds)
This one should be called fd.
> @@ -128,8 +128,8 @@ class WXDLLIMPEXP_BASE wxSortedArrayString : public wxSortedArrayStringBase
{
reserve(src.size());
- for ( size_t n = 0; n < src.size(); n++ )
- Add(src[n]);
+ for (const auto & n : src)
There is a real problem with the loop variable names. This one should be called s or str, not n.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Nong Thi Quynh Anh
anhnong90 left a comment (wxWidgets/wxWidgets#26640)Yes, this was the automated behavior of clang-tidy, which unfortunately generated poor variable names. I ran clang-tidy with the modernize-loop-convert checker across the entire repository (src/, include/, interface/, and tests/). In the samples/ directory, clang-tidy found no additional loops requiring modification beyond my initial manual changes.
I am currently reviewing all modified files to manually rename the loop variables according to the codebase conventions.
Thank you for the feedback!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Nong Thi Quynh Anh
@anhnong90 pushed 3 commits.
- 675b7c4 Use range-based for loops in dialup and mediaplayer samples
- 5ab606e Modernize loops across core source files using clang-tidy
- ba7183a Modernize safe for-loops; revert unsafe internal containers
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Nong Thi Quynh Anh
anhnong90 left a comment (wxWidgets/wxWidgets#26640)The modernized loop conversions compile successfully locally, and local test execution passes 100% cleanly when using a stable display server backend (GDK_BACKEND=x11).
The three GUI pipeline failures on Grid::Size are known spurious UI automation bugs tracked under Upstream Issue #25779. The CI output logs explicitly show a warning that a timed-out waiting for mouse release to be processed, which means the simulated column-drag actions timed out or were swallowed by the virtual cloud displays before the layout metrics updated. Could you please take a look, @vadz?
Thank you!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26640)Thanks, I'll have a look at it but there will be some delay, sorry.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 9a770d4 Don't compile the loop containing wxASSERT when it does nothing
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26640)FYI, I've pushed a small fix for the 2 real CI failures.
If/when the CI builds pass, I'll look the changes over one more time and merge them, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.