Use range iterators for RBSet in most cases

[AaronRecord]

See #51409, #50284, #50511

I adapted and refactored the original python script a little, and then proceeded to make some manual changes for some of the more tricky cases that were faster to just correct afterwards.

script

import os
import re


DIRECTORY = "C:/Users/Aaron/Projects/GodotSource/godot"
EXTENSIONS = ("cpp", "h", "hpp")

def only_const_usage() -> bool:
	assigned_to_regex: str = r"[^\s]+? [&*]?[_a-zA-Z][_a-zA-Z0-9]* = &?(Object::cast_to<.+?>\()?" + re.escape(iterator_name) + r"(((\.|->).+?)+)?;"
	iterated_through_regex: str = r"\([^\s]+? &[_a-zA-Z][_a-zA-Z0-9]* : " + re.escape(iterator_name) + r"\)"

	# Assigned to a constant value, must be constant.
	if re.search(f"const {assigned_to_regex}", block_text) != None:
		return True
	if re.search(f"const {iterated_through_regex}", block_text) != None:
		return True

	checks = [
				# Value or sub-value is assigned.
				iterator_name + r"(((\.|->).+?)+)?(\[.+?\])? = .+?;",
				# Non-const method called.
				iterator_name + r"(((\.|->).+?)+)?(->|\.)((update|invalidate)\(|(write)\[)",
			]

	for check in [re.compile(x) for x in checks]:
		if check.search(block_text) != None:
			return False

	checks = [assigned_to_regex, iterated_through_regex]

	if is_ptr:
		# If the value is assigned to a non-const pointer.
		for check in [re.compile(x) for x in checks]:
			if check.search(block_text) != None:
				return False

	return True


os.chdir(DIRECTORY)

# Get a list of files to modify.
grabbed_files: list[str] = []
for root, dirs, files in os.walk(DIRECTORY, topdown=True):
	dirs[:] = [d for d in dirs if d not in (".git", "thirdparty")]
	for f in files:
		if not f.endswith(EXTENSIONS) or ".gen" in f:
			continue

		grabbed_files.append(os.path.join(root, f))


for_loop_regex = re.compile(r"for ?\((const )?RBSet<(.+?)>::Element \*(.+?) = (.+?)\.front\(\); \3; \3 = \3->next\(\)\) \{")
for grabbed_file in grabbed_files:
	# Read in the file.
	with open(grabbed_file, 'r', encoding="utf-8", newline="\n") as f:
		new_file_text = f.read()

	pos = 0
	while result := for_loop_regex.search(new_file_text, pos):
		pos = result.end(0)

		# e.g.
		# `for (RBSet<String>::Element *E = current->files.front(); E; E = E->next()) {` ->
		# `for (const String &E : current->files) {`
		is_const: bool = result[1] != None # False
		value_type: str = result[2] # "String"
		iterator_name: str = result[3 	] # "E"
		container_name: str = result[4] #"current->files"

		is_ptr: bool = value_type[-1] == "*" # False

		# Get closing bracket position.
		bracket_count = 1
		bracket_pos = result.end(0)
		while bracket_count > 0:
			match new_file_text[bracket_pos]:
				case "{":
					bracket_count += 1
				case "}":
					bracket_count -= 1

			bracket_pos += 1

		block_text = new_file_text[result.end(0):bracket_pos]
		if re.search(r"\b" + re.escape(iterator_name) + r"->next\(\)", block_text):
			# Skip iterators that make use of `->next()`.
			continue
		if re.search(r"\b" + re.escape(container_name) + r"(->|\.)erase\(", block_text):
			# Container is modified, I'm not sure how this is handled.
			continue

		block_text = re.sub(
			r"(?<=\b)" + re.escape(iterator_name) + r"->get\(\)",
			iterator_name,
			block_text
		)

		# Try to convert iterators that weren't previously const to const if possible.
		if not is_const:
			is_const = only_const_usage()

		for_loop_text = f"for ({'const ' if is_const and not value_type.startswith('const ') else ''}{value_type}{' &' if not is_ptr else ''}{iterator_name} : {container_name}) {{"
		new_file_text = new_file_text[:result.start(0)] + for_loop_text + block_text + new_file_text[bracket_pos:]

	if pos == 0:
		# `pos` hasn't moved which means no matches were found.
		# There's no need to write back the file if nothing's different.
		continue

	# Write back to the file.
	with open(grabbed_file, 'w', encoding="utf-8", newline="\n") as f:
		f.write(new_file_text)

[AaronRecord]

Ideally stuff like this should be this:

Suggested change

	for (Material *E : shader->owners) {
	Material *material = E;
	for (Material *material : shader->owners) {

Not sure if I should bother to try and fix all the instances where something like this happens in this PR though. I think it'd be better suited for a future PR, checking for all types range iterators.

[akien-mga]

I guess that could be a follow-up PR yeah, as long as it's not too hard to find those spots after the pact. Maybe grepping for = E; can help find such locations.

[akien-mga]

Looks good to me.

I assume the manual fixes in the second commit are actually needed for the first commit to compile properly? If so they should likely be squashed before merge, so that we keep a merged history where all commits can be compiled (important when bisecting). But that can be done at the last minute once we're 100% sure that the script-made changes are good (seems so to me but maybe @reduz wants to give it a quick look too).

[reduz]

Awesome work @LightningAA, you are my hero!

[akien-mga]

I assume the manual fixes in the second commit are actually needed for the first commit to compile properly? If so they should likely be squashed before merge, so that we keep a merged history where all commits can be compiled (important when bisecting). But that can be done at the last minute once we're 100% sure that the script-made changes are good (seems so to me but maybe @reduz wants to give it a quick look too).

I pushed this as an update now so we can merge it right away (as I assume from your timezone that you must be sound asleep right now :P).

[akien-mga]

Thanks!