Replace every use of the foreach macro with a C++11 range-based for loop #2669

Fastigium · 2016-03-11T15:56:00Z

Feedback on the CMakeLists.txt changes, please! I don't know how to do that right. Plus things aren't const correct everywhere. Otherwise, this PR should be as good as #2658, and even doesn't exhibit #2659 😊

Main commit message:

This prevents a race condition with Qt5. A foreach loop makes a copy of its
Qt container, increasing the reference count to the container's internal
data. Qt5 often asserts isDetached(), which requires the reference count to
be <= 1. This assertion fails when the foreach loop increases the reference
count at exactly the wrong moment. Using a range-based for loop prevents an
unnecessary copy from being made and ensures this race condition isn't
triggered.

michaelgregorius · 2016-03-11T16:05:12Z

include/InstrumentPlayHandle.h

@@ -56,7 +56,7 @@ class EXPORT InstrumentPlayHandle : public PlayHandle
 		do
 		{
 			nphsLeft = false;
-			foreach( const NotePlayHandle * cnph, nphv )
+			for( const NotePlayHandle * cnph : nphv )


I would rename cnph to something like notePlayHandle to make things more readable in the loop.

michaelgregorius · 2016-03-11T16:13:00Z

@Fastigium I have left some remarks. Looks good to me otherwise. Thanks for your efforts!

Fastigium · 2016-03-11T18:08:41Z

@michaelgregorius Thanks, I adapted include/InstrumentPlayHandle.h to incorporate your suggestions.

And I also noticed that the Windows builds didn't work anymore in Travis. Apparently switching to -std=c++0x for all code gives errors there. Suggestions/help appreciated!

tresf · 2016-03-11T18:33:34Z

Apparently switching to -std=c++0x for all code gives errors there. Suggestions/help appreciated!

@Fastigium are you sure it's needed? We already do that here: https://github.com/LMMS/lmms/blob/master/src/CMakeLists.txt#L11

Fastigium · 2016-03-11T18:44:23Z

@tresf It won't compile otherwise, as I replaced some foreach loops in plugins/. But it turns out I broke something else as well while incorporating michaelgregorius's suggestions, let me look at that...

michaelgregorius · 2016-03-11T18:45:45Z

When I compile my own projects under Windows with MinGW I have to add the following snippet to my CMakeLists.txt:

if(WIN32)
add_definitions(-D_USE_MATH_DEFINES)
endif(WIN32)

Otherwise the compiler has problems with constants from the <cmath> include, e.g. M_PI. Seems like the Travis build might have the same problem.

I now wonder whether the ADD_DEFINITIONS(-std=c++0x) referenced by @tresf does some extra magic behinds the scenes to automatically add -D_USE_MATH_DEFINES for Windows builds.

tresf · 2016-03-11T18:48:52Z

@tresf It won't compile otherwise, as I replaced some foreach loops in plugins/. But it turns out I broke something else as well while incorporating michaelgregorius's suggestions, let me look at that...

Then add it to the affected plugins. 👍

tresf · 2016-03-11T18:49:24Z

Or optionally... leave the plugin fixing to the plugin authors 😆

Fastigium · 2016-03-11T18:50:45Z

That could take a while :P. I'll try adding it to the affected plugins.

tresf · 2016-03-11T18:53:21Z

That could take a while :P. I'll try adding it to the affected plugins.

This is how I did it for FLP import:

2035ff3#diff-52ceeeebf8bf77187338a01c3b994ecdR6

tresf · 2016-03-11T18:54:34Z

I now wonder whether the ADD_DEFINITIONS(-std=c++0x) referenced by @tresf does some extra magic behinds the scenes to automatically add -D_USE_MATH_DEFINES for Windows builds.

I'm guessing cmake helps out per http://stackoverflow.com/a/16144116/3196753

michaelgregorius · 2016-03-11T18:56:16Z

I think the builds break because you simply replaced

foreach( const NotePlayHandle * cnph, nphv )

with

for( NotePlayHandle * notePlayHandle : nphv )

In the new version the NotePlayHandle is not const. However, the collection nphv still contains NotePlayHandles which are const because it is a ConstNotePlayHandleList. This list is fetched using the method NotePlayHandle::nphsOfInstrumentTrack. To make the code work you would have to add a similar method which does not return a ConstNotePlayHandleList but a NotePlayHandleList (both defined in the header of NotePlayHandle.h).

Fastigium · 2016-03-11T18:59:01Z

@michaelgregorius Yeah, I just rebased the const_cast back in minutes ago. I think I'll leave it there for now. That part of InstrumentPlayHandle.h should be removed or otherwise fixed in the near future anyway per #2606 (on which feedback is still welcome, btw). Thanks for taking a look at it nevertheless!

Fastigium · 2016-03-11T19:27:03Z

Ok, rebased to only ADD_DEFINITIONS(-std=c++0x) for plugins that require it. Windows builds still seem to fail. Hm.

@tresf Should I include that IF(LMMS_BUILD_CLANG) block everywhere I put ADD_DEFINITIONS(-std=c++0x)?

tresf · 2016-03-11T19:57:14Z

@tresf Should I include that IF(LMMS_BUILD_CLANG) block everywhere I put ADD_DEFINITIONS(-std=c++0x)?

Well, this was recently changed to LMMS_BUILD_APPLE because it broke the clang compiler on non-Macs, but I guess it depends how Travis reacts. If it breaks on my older build machines, I can add it at release time.

M_PI is no longer defined by default in C++11, but lb302.cpp needs it. Therefore, before switching to C++11, we add an include.

This prevents a race condition with Qt5. A foreach loop makes a copy of its Qt container, increasing the reference count to the container's internal data. Qt5 often asserts isDetached(), which requires the reference count to be <= 1. This assertion fails when the foreach loop increases the reference count at exactly the wrong moment. Using a range-based for loop prevents an unnecessary copy from being made and ensures this race condition isn't triggered.

Fastigium · 2016-03-13T15:33:39Z

Right, after playing ping pong with Travis for a few hours, this PR is finally all green! I also performed the move-channels-while-playing-RandomProjectNumber14253-test again with no crashes, and #2659 is still absent as well.

Can anyone look over the rebased code once more? Only the first two commits were changed.

tresf · 2016-03-13T15:37:39Z

plugins/opl2/CMakeLists.txt

@@ -1,3 +1,6 @@
 INCLUDE(BuildPlugin)

+# Enable C++11
+SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")


I assume ADD_DEFINITIONS didn't work here?

Correct, it triggers a warning that it's an invalid option for C files

Fastigium · 2016-03-14T13:15:40Z

I'm going to go ahead and merge this. I have a PR ready to fix FX channel removal crashes, and it would benefit the testing of that one a lot if no random race conditions got in the way. Thanks for all the feedback, and if I'm too brash merging this on my own, please let me know.

Replace every use of the foreach macro with a C++11 range-based for loop

zonkmachine · 2016-03-14T18:02:15Z

And, all of a sudden, everything just works...

Fastigium mentioned this pull request Mar 11, 2016

Replace every use of the foreach macro with an iterator loop #2658

Closed

michaelgregorius reviewed Mar 11, 2016
View reviewed changes

Fastigium force-pushed the foreach2for branch from d2385c0 to 5a5e00b Compare March 11, 2016 18:05

Fastigium force-pushed the foreach2for branch from 5a5e00b to dc7798e Compare March 11, 2016 18:48

Fastigium force-pushed the foreach2for branch from dc7798e to da13518 Compare March 11, 2016 19:17

Fastigium added 3 commits March 13, 2016 16:08

Make lb302 include math.h so we can switch it to C++11

bfa83da

M_PI is no longer defined by default in C++11, but lb302.cpp needs it. Therefore, before switching to C++11, we add an include.

Compile several plugins with -std=c++0x to support range-based for loops

ac67f2a

Fastigium force-pushed the foreach2for branch from 934a80b to 3c7bfba Compare March 13, 2016 15:10

tresf reviewed Mar 13, 2016
View reviewed changes

Fastigium added a commit that referenced this pull request Mar 14, 2016

Merge pull request #2669 from Fastigium/foreach2for

9e98a16

Replace every use of the foreach macro with a C++11 range-based for loop

Fastigium merged commit 9e98a16 into LMMS:master Mar 14, 2016

Fastigium mentioned this pull request Mar 14, 2016

Fix crashes on deleting an FX channel #2675

Merged

Umcaruje mentioned this pull request Mar 17, 2016

Random crashes while playing #2192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace every use of the foreach macro with a C++11 range-based for loop #2669

Replace every use of the foreach macro with a C++11 range-based for loop #2669

Fastigium commented Mar 11, 2016

michaelgregorius Mar 11, 2016

michaelgregorius commented Mar 11, 2016

Fastigium commented Mar 11, 2016

tresf commented Mar 11, 2016

Fastigium commented Mar 11, 2016

michaelgregorius commented Mar 11, 2016

tresf commented Mar 11, 2016

tresf commented Mar 11, 2016

Fastigium commented Mar 11, 2016

tresf commented Mar 11, 2016

tresf commented Mar 11, 2016

michaelgregorius commented Mar 11, 2016

Fastigium commented Mar 11, 2016

Fastigium commented Mar 11, 2016

tresf commented Mar 11, 2016

Fastigium commented Mar 13, 2016

tresf Mar 13, 2016

Fastigium Mar 13, 2016

tresf Mar 13, 2016

Fastigium commented Mar 14, 2016

zonkmachine commented Mar 14, 2016

Replace every use of the foreach macro with a C++11 range-based for loop #2669

Replace every use of the foreach macro with a C++11 range-based for loop #2669

Conversation

Fastigium commented Mar 11, 2016

michaelgregorius Mar 11, 2016

Choose a reason for hiding this comment

michaelgregorius commented Mar 11, 2016

Fastigium commented Mar 11, 2016

tresf commented Mar 11, 2016

Fastigium commented Mar 11, 2016

michaelgregorius commented Mar 11, 2016

tresf commented Mar 11, 2016

tresf commented Mar 11, 2016

Fastigium commented Mar 11, 2016

tresf commented Mar 11, 2016

tresf commented Mar 11, 2016

michaelgregorius commented Mar 11, 2016

Fastigium commented Mar 11, 2016

Fastigium commented Mar 11, 2016

tresf commented Mar 11, 2016

Fastigium commented Mar 13, 2016

tresf Mar 13, 2016

Choose a reason for hiding this comment

Fastigium Mar 13, 2016

Choose a reason for hiding this comment

tresf Mar 13, 2016

Choose a reason for hiding this comment

Fastigium commented Mar 14, 2016

zonkmachine commented Mar 14, 2016