Migration to C++17 code refactor for loops #18559

GermanAizek · 2023-12-15T16:08:55Z

@hrydgard Possible improvement codegeneration by compiler, improvement readability, reduction code size.

hrydgard · 2023-12-16T21:19:53Z

The build is failing on Windows for some reason. Take a look at the build logs and fix it up please, then I'll take a look.

GPU/Common/TextureReplacer.cpp

hrydgard

Please configure tabs in your editor correctly.

hrydgard · 2023-12-18T21:37:00Z

Common/Serialize/SerializeMap.h

-		for (auto it = x.begin(), end = x.end(); it != end; ++it) {
-			if (it->second != nullptr)
-				delete it->second;
+		for (const auto &[_,value] : x) {


Wow, this is getting a little to modern for me haha.

fix it 37ce2a4

hrydgard · 2023-12-18T21:37:09Z

Common/System/OSD.cpp

+                    entry.progress = entry.maxValue;
 				} else {
 					// Fake a full progress
-					iter->minValue = 0;
-					iter->maxValue = 1;
-					iter->progress = 1;
+                    entry.minValue = 0;
+                    entry.maxValue = 1;
+                    entry.progress = 1;


Tabs are wrong

fix it 37ce2a4

hrydgard · 2023-12-18T21:37:16Z

Common/System/OSD.cpp

 				}
 			}
-			iter->endTime = time_now_d() + delay_s + FadeoutTime();
+            entry.endTime = time_now_d() + delay_s + FadeoutTime();


fix it 37ce2a4

hrydgard · 2023-12-18T21:37:30Z

Common/UI/Root.cpp

-			heldKeys.insert(hk);
+			HeldKey k = hk;
+			heldKeys.erase(k);
+            k.triggerTime = now + repeatInterval;


fix it 37ce2a4

hrydgard · 2023-12-18T21:37:42Z

Common/UI/Screen.cpp

-	for (auto iter = stack_.begin(); iter != stack_.end(); ++iter) {
-		iter->screen->resized();
+	for (auto &layer : stack_) {
+        layer.screen->resized();


fix it 37ce2a4

… C++17 and replaced on structured binding map C++17

…structured binding map C++17

hrydgard · 2023-12-20T10:00:39Z

There's a conflict now.

hrydgard · 2023-12-20T10:57:09Z

still a conflict

GermanAizek · 2023-12-20T11:23:04Z

I don't understand how to fix it, I did same as in for-based-loop-cpp17 (range-based-loop-cpp17) branch with added emplace from master

anr2me · 2023-12-26T12:46:24Z

You need to choose one version(based on the branch name) of the change to resolve this conflict

either removes the code between <<<<<...===== or the code between =====...>>>>> (you can see the branch name on the right side of the arrows)

unknownbrackets · 2023-12-29T04:59:35Z

Common/Input/InputState.cpp

@@ -44,8 +44,8 @@ static std::unordered_map<InputDeviceID, int> uiFlipAnalogY;

 static void AppendKeys(std::vector<InputMapping> &keys, const std::vector<InputMapping> &newKeys) {
 	keys.reserve(newKeys.size());


I guess this should actually be keys.reserve(keys.size() + newKeys.size());. If it actually even helps perf.

Any reason not to use const auto &?

-[Unknown]

fix it 4ce7077

unknownbrackets · 2023-12-29T05:00:15Z

Common/Serialize/SerializeList.h

-	typename std::list<T>::iterator itr, end;
-	for (itr = x.begin(), end = x.end(); itr != end; ++itr)
-		Do(p, *itr);
+	for (T elem : x)


It's actually important that this does not perform a copy, as it did not before.

-[Unknown]

fix it here 4ce7077

unknownbrackets · 2023-12-29T05:02:33Z

Common/Serialize/SerializeMap.h

+		for (const auto &[_, value] : x) {
+			if (value != nullptr)
+				delete value;


nit: Personally, I like to always pretend that delete mutates a pointer, and avoid const for things I'm going to delete. Semantically, I think it'd be confusing to see a const int *p argument to a function which ends up actually deleting that pointer.

Anyway, it sometimes does in fact mutate the object by calling the destructor.

-[Unknown]

If I'm not mistaken, delete works fine with the nullptr so the check is superfluous.

unknownbrackets · 2023-12-29T05:05:13Z

Common/UI/Root.cpp

 			// Cannot modify the current item when looping over a set, so let's do this instead.
-			HeldKey hk = *iter;
-			heldKeys.erase(hk);
-			hk.triggerTime = now + repeatInterval;
-			heldKeys.insert(hk);
+			HeldKey k = hk;
+			heldKeys.erase(k);
+			k.triggerTime = now + repeatInterval;
+			heldKeys.insert(k);
 			goto restart;


I always worry about these goto-restart things with range-fors. It's less clear what tricks the compiler might do and whether it's safe. Maybe this should be a std::remove_if pattern or something.

-[Unknown]

revert it here 4ce7077

Core/HLE/proAdhocServer.cpp

GermanAizek · 2024-04-01T14:17:30Z

@hrydgard,
Oh, I thought this PR had been merged for a long time, how do I fix conflicts now?

#18559 (comment)
I think requirement fixed missing space is not important

…heckins@unknownbrackets.org> Signed-off-by: Unknown W. Brackets <checkins@unknownbrackets.org> Co-authored-by: Unknown W. Brackets <checkins@unknownbrackets.org>

GermanAizek · 2024-04-01T14:20:55Z

@hrydgard, and I noticed that in github I can add commit to PR on Web version, it's convenient. Done.

Nemoumbra · 2024-04-01T14:31:55Z

Oh, I thought this PR had been merged for a long time

You have not resolved 2 conversations with Unknown regarding your changes, apparently the one in SerializeList.h is very important.

GermanAizek · 2024-04-12T11:09:00Z

@hrydgard, im fixed all conflicts and add fixes Unknown to commit.

Nemoumbra reviewed Dec 17, 2023

View reviewed changes

GPU/Common/TextureReplacer.cpp Outdated Show resolved Hide resolved

GermanAizek force-pushed the for-based-loop-cpp17 branch from 3f840a7 to 63337c5 Compare December 18, 2023 15:10

hrydgard requested changes Dec 18, 2023

View reviewed changes

GermanAizek added 2 commits December 20, 2023 12:09

[Common/Data/File/Input/Net/Serialize/System/UI] Using for based loop…

37ce2a4

… C++17 and replaced on structured binding map C++17

[Core/HLE/GPU/D3D11/GLES] Using for based loop C++17 and replaced on …

487d55c

…structured binding map C++17

GermanAizek force-pushed the for-based-loop-cpp17 branch from 63337c5 to 487d55c Compare December 20, 2023 09:11

Fixed merge conflict

e14cf8c

GermanAizek force-pushed the for-based-loop-cpp17 branch from 49124dd to e14cf8c Compare December 20, 2023 10:25

Merge branch 'master' into for-based-loop-cpp17

2d75803

unknownbrackets reviewed Dec 29, 2023

View reviewed changes

Fixed missing space Core/HLE/proAdhocServer by Unknown W. Brackets <c…

2467a54

…heckins@unknownbrackets.org> Signed-off-by: Unknown W. Brackets <checkins@unknownbrackets.org> Co-authored-by: Unknown W. Brackets <checkins@unknownbrackets.org>

GermanAizek added 2 commits April 12, 2024 14:03

[Common] Fixes from Unknown W. Brackets: <checkins@unknownbrackets.org>

4ce7077

Merge branch 'master' into for-based-loop-cpp17

e6f8be1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration to C++17 code refactor for loops #18559

Migration to C++17 code refactor for loops #18559

GermanAizek commented Dec 15, 2023

hrydgard commented Dec 16, 2023

hrydgard left a comment

hrydgard Dec 18, 2023

GermanAizek Dec 20, 2023

hrydgard Dec 18, 2023

GermanAizek Dec 20, 2023

hrydgard Dec 18, 2023

GermanAizek Dec 20, 2023

hrydgard Dec 18, 2023

GermanAizek Dec 20, 2023

hrydgard Dec 18, 2023

GermanAizek Dec 20, 2023

hrydgard commented Dec 20, 2023

hrydgard commented Dec 20, 2023

GermanAizek commented Dec 20, 2023

anr2me commented Dec 26, 2023

unknownbrackets Dec 29, 2023

GermanAizek Apr 12, 2024

unknownbrackets Dec 29, 2023

GermanAizek Apr 12, 2024

unknownbrackets Dec 29, 2023

Nemoumbra Dec 29, 2023

unknownbrackets Dec 29, 2023

GermanAizek Apr 12, 2024 •

edited

GermanAizek commented Apr 1, 2024

GermanAizek commented Apr 1, 2024 •

edited

Nemoumbra commented Apr 1, 2024 •

edited

GermanAizek commented Apr 12, 2024

		@@ -44,8 +44,8 @@ static std::unordered_map<InputDeviceID, int> uiFlipAnalogY;

		static void AppendKeys(std::vector<InputMapping> &keys, const std::vector<InputMapping> &newKeys) {
		keys.reserve(newKeys.size());

Migration to C++17 code refactor for loops #18559

Are you sure you want to change the base?

Migration to C++17 code refactor for loops #18559

Conversation

GermanAizek commented Dec 15, 2023

hrydgard commented Dec 16, 2023

hrydgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrydgard commented Dec 20, 2023

hrydgard commented Dec 20, 2023

GermanAizek commented Dec 20, 2023

anr2me commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GermanAizek Apr 12, 2024 • edited

Choose a reason for hiding this comment

GermanAizek commented Apr 1, 2024

GermanAizek commented Apr 1, 2024 • edited

Nemoumbra commented Apr 1, 2024 • edited

GermanAizek commented Apr 12, 2024

GermanAizek Apr 12, 2024 •

edited

GermanAizek commented Apr 1, 2024 •

edited

Nemoumbra commented Apr 1, 2024 •

edited