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

Change GDScriptDataType container_element_type to vector container #81662

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Sep 14, 2023

Gonna start extracting bits from my typed dictionary PR (#78656) so the changes can be integrated more smoothly, starting with changing container_element_type from a pointer to a vector of pointers. This allows support for multiple types in the container syntax, without drastically overhauling the current implementations

While mostly superfluous in our current context of only having typed arrays, this will allow other type collections (eg: dictionaries) to be integrated straight away without needing to change the existing system. It also adds support for highlighting all container elements as types, and will recognize if a container is over/under capacity (only for arrays atm, will specify it requires exactly one type)

@Repiteo Repiteo requested a review from a team as a code owner September 14, 2023 18:41
@AThousandShips AThousandShips added this to the 4.x milestone Sep 14, 2023
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_function.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_function.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_function.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
@adamscott
Copy link
Member

Decision made at the GDScript Team Meeting that we want this PR to make it for 4.2, even if there's nothing front-facing to a user, will take time to review it this week. This PR will facilitate future development of typed dictionaries.

modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_function.h Outdated Show resolved Hide resolved
@Repiteo Repiteo force-pushed the container-type-vector branch 2 times, most recently from cbbedc0 to 35adb70 Compare September 26, 2023 17:45
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall, just a couple of nitpicks.

modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
@Repiteo Repiteo force-pushed the container-type-vector branch 3 times, most recently from d90e809 to 5b1e3fb Compare September 28, 2023 20:36
@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 2, 2023
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_analyzer.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
@Repiteo Repiteo force-pushed the container-type-vector branch 2 times, most recently from 812f355 to 67e8d72 Compare October 5, 2023 17:01
@Repiteo Repiteo mentioned this pull request Oct 7, 2023
10 tasks
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
@Repiteo
Copy link
Contributor Author

Repiteo commented Dec 2, 2023

Haven't seen/heard anything to the contrary, so the containers are now changed to be public & not use pointers. This certainly simplified the code, with several areas being made redundant in the process & cleaned up as a result

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vnen
Copy link
Member

vnen commented Dec 4, 2023

Regarding the pointer, as the comment said it was for memory management. I didn't to construct and copy an empty container type every time, since it's more likely that it does not exist. Being a pointer it can be set only when needed, which is also why it was private.

With a vector this is less of a concern because the vector can be empty.

@akien-mga akien-mga changed the title container_element_type to vector container Change GDScriptDataType container_element_type to vector container Dec 4, 2023
modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.h Outdated Show resolved Hide resolved
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved during the GDScript meeting, just have to apply the suggestions by @dalexeev. Thanks!

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Repiteo !

@YuriSizov YuriSizov merged commit 3b9347d into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@Repiteo Repiteo deleted the container-type-vector branch December 9, 2023 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants