Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor old API #11526

Open
YYF233333 opened this issue Jan 9, 2025 · 0 comments
Open

Refactor old API #11526

YYF233333 opened this issue Jan 9, 2025 · 0 comments

Comments

@YYF233333
Copy link

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

As mentioned in godotengine/godot#99936, I am attempting to replace List with LocalVector. During this process, I discovered many old APIs. The modification history often looks like this:

Git History of core/object/object.cpp at line 469 on master by all authors 📊 

A Whole New World (clang-format edition) 
5dbf1809c6 by Rémi Verschelde <[email protected]>, Sun Mar 5 23:44:50 2017

GODOT IS OPEN SOURCE 
0b806ee0fc by Juan Linietsky <[email protected]>, Mon Feb 10 09:10:30 2014

These old API signatures have not undergone any substantial changes since Godot became open-source. Their main issues include:

  • List<>*: When references can be used, they should be preferred over pointers. Since these List objects are clearly return values, they cannot be null. All these pointers do is open the door to null pointer dereferences and foster distrust between the caller and callee.

  • Return values as function parameters: This approach might have been useful before C++17, but modern C++ compilers generally apply Return Value Optimization (RVO) effectively (as discussed in Use LocalVector instead of List as arg of Dictionary::get_key_list godot#100333). Therefore, it no longer provides performance benefits (given that these APIs haven’t been modified in 11 years, they are likely part of a cold path). Instead, this approach introduces readability issues. The following pattern is common in the codebase:

List<String> container;
something.get_xxx_list(&container);
for (const String &E : container) {
    ...
}

If the body of the for loop is lengthy or the code is deeply nested, ensuring that container is not referenced later becomes a cumbersome task. In fact, I’ve encountered examples where the container is referenced hundreds of lines later. If the container were returned as a value, we could write:

for (const String &E : something.get_xxx_list()) {
    ...
}

This eliminates the need to worry about container being referenced within or after the loop, making refactoring much easier.

  • Double Iteration: Same example above, in cases where the API is backed by a Vector, HashMap, etc., the get_xxx_list method first iterates over that container to create a List, and then the caller iterates over this List. This results in an additional iteration. For HashMap, where the caller often needs both keys and values, the get_list API typically only returns the keys, forcing the caller to perform an additional lookup.

    While sufficiently high optimization levels might be able to eliminate this redundant iteration, there seems to be no standard guarantee for this behavior. If we directly returned an iterator instead, the process would become much simpler. The only downside would likely be longer function signatures. :)

Describe the feature / enhancement and how it helps to overcome the problem or limitation

See above.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I'll open a PR once we reach a consensus.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

It is godot source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants