Core: Migrate typed container logic #96741
Open
+311
−703
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Repiteo
force-pushed
the
core/typed-container-migration
branch
from
2ddb82f
to
ab869ab
Compare
|
Made another minor addition because I couldn't justify an entirely separate PR for it: |
RadiantUwU
approved these changes
Sep 11, 2024
| @@ -41,8 +41,7 @@ | |||
| #include "core/variant/dictionary.h" | |||
| #include "core/variant/variant.h" | |||
|
|
|||
| class ArrayPrivate { | |||
| public: | |||
| struct Array::ArrayPrivate { | |||
There was a problem hiding this comment.
OK, Private collection headers moved inside their collections.
| @@ -199,4 +199,13 @@ class Array { | |||
| ~Array(); | |||
| }; | |||
|
|
|||
| template <typename T> | |||
| class TypedArray : public Array { | |||
There was a problem hiding this comment.
Typed collections have been moved inside their generic collection headers.
| // This is for typed containers. | ||
|
|
||
| template <typename T> | ||
| struct PtrToArg<TypedArray<T>> { |
There was a problem hiding this comment.
Variants can now be casted directly to their internal typed collections.
Repiteo
force-pushed
the
core/typed-container-migration
branch
3 times, most recently
from
3295642
to
c63cf08
Compare
Repiteo
force-pushed
the
core/typed-container-migration
branch
from
c63cf08
to
0ab9667
Compare
Repiteo
force-pushed
the
core/typed-container-migration
branch
from
0ab9667
to
0add01b
Compare
• Dedicated typed container files entirely removed • Typed container classes now defined in their base headers; methods declared in `variant.h` • Consolidate to a single template for both typed containers, thanks to `constexpr` • Nest private containers in their base classes
Repiteo
force-pushed
the
core/typed-container-migration
branch
from
0add01b
to
0bbc059
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
type_infoproperties to detect & identify Variant types #88567This PR aims to significantly streamline the use of typed arrays/dictionaries across the repo by making their include chains much more reasonable. Instead of requiring dedicated files, they're instead declared right in the base files for each respective container. The templated logic is handled in
variant.h, similar to how it's already handling array iterator functions. Because everything is now consolidated into a single location, this entirely removes the need to forward-declare the structs in various classes or require including the typed header with extra baggage; within reason, anything can just use typed containers right away.As part of the transition, this PR also integrates the ideas originally conceived in #88567: consolidating the typed container templates to a single instance. This was done with
constexprto allow for conditional checks right in the templates themselves, but the biggest improvement since then was to handle this logic invariant.hrather thantype_info.h. While I still fully believe that a refactor oftype_info.his an inevitability in the near-ish future, that's not required for this scenario that only concerns itself with mapping types toVariant::Type.The main other thing this PR does is explicitly disallow
TypedArray<Variant>andTypedDictionary<Variant, Variant>by declaring them as undefined behavior. This won't likely be something that we stick with long-term, but for now their specifics simply haven't been discussed and, unlike GDScript, they would behave differently than the base containers. Whether this will have an actual impact is anyone's guess, but this was already adding static conditional logic to a system that was fundamentally incompatible with it previously, so no harm done.