Use const references where possible for List range iterators #50809

akien-mga · 2021-07-24T16:26:12Z

Follow-up to #50511.

akien-mga · 2021-07-24T20:18:04Z

core/io/resource_format_binary.cpp

-		for (Property &F : rd.properties) {
-			Property &p = F;
+		for (const Property &p : rd.properties) {


Changed this as it seemed redundant.

akien-mga · 2021-07-24T20:21:03Z

drivers/vulkan/rendering_device_vulkan.cpp

@@ -8542,7 +8542,7 @@ void RenderingDeviceVulkan::_free_rids(T &p_owner, const char *p_type) {
 		} else {
 			WARN_PRINT(vformat("%d RIDs of type \"%s\" were leaked.", owned.size(), p_type));
 		}
-		for (RID E : owned) {
+		for (const RID &E : owned) {


Turned all for (RID foo : bar) into for (const RID &foo : bar).

I didn't double check the implications but @reduz also seemed to think it's fine.

Yeah, const reference is better here.

editor/plugins/canvas_item_editor_plugin.cpp

akien-mga · 2021-07-24T20:25:26Z

editor/scene_tree_dock.cpp

-		for (Object::Connection &F : cl) {
-			Object::Connection &c = F;
+		for (const Object::Connection &c : cl) {


Simplified.

modules/gdscript/gdscript_analyzer.cpp

akien-mga · 2021-07-24T20:28:54Z

modules/gdscript/gdscript_parser.h

@@ -1431,7 +1431,7 @@ class GDScriptParser {
 		void push_line(const String &p_line = String());
 		void push_text(const String &p_text);

-		void print_annotation(AnnotationNode *p_annotation);
+		void print_annotation(const AnnotationNode *p_annotation);


I made this change to allow compiling the other changes iterating with const AnnotationNode *E, and since the method doesn't seem to modify the node.

But I see now it makes it inconsistent with other print_* methods, I could change it back if needed @vnen.

akien-mga · 2021-07-24T20:29:28Z

modules/gdscript/language_server/gdscript_text_document.cpp

-		for (const lsp::DocumentSymbol *&E : list) {
-			if (const lsp::DocumentSymbol *s = E) {
-				contents.push_back(s->render().value);
-			}
+		for (const lsp::DocumentSymbol *s : list) {
+			contents.push_back(s->render().value);


I think the code should be equivalent?

I don't think those are equivalent. The = operator returns the value of the newly assigned variable. So the original code is assigning and checking for nullptr at the same time. So with the new code we may be accessing an null pointer.

scene/3d/node_3d.cpp

scene/3d/sprite_3d.cpp

scene/main/node.cpp

akien-mga · 2021-07-24T20:34:09Z

Some stuff I noticed while working on this:

We have many occurrences of for (Ref<T> foo : bar) which should probably be converted to for (const Ref<T> &foo : bar). That will be another PR.
const T *foo is often trickier as we tend to use the pointer in a non-const way in most cases. That's why there's only a handful of const specifiers added to pointer iterators here.

akien-mga · 2021-07-24T21:26:32Z

Updated to leave the for (T *&E) cases alone after discussion with @JFonS. There aren't many of them but if we confirm that their use is intentional, we can indeed add parentheses to clarify (they're easy to find by grepping for *& outside thirdparty code).

akien-mga added enhancement topic:core labels Jul 24, 2021

akien-mga added this to the 4.0 milestone Jul 24, 2021

akien-mga requested review from aaronfranke and reduz July 24, 2021 16:26

akien-mga requested review from a team as code owners July 24, 2021 16:26

akien-mga force-pushed the iterators-const-references branch 3 times, most recently from 9783cdb to 5051c57 Compare July 24, 2021 20:05