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

Fix Array & Dictionary copy constructor #69117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Nov 24, 2022

The description states:

Constructs an Array as a copy of the given Array.

Case and point:
image

And if you do want a copy, Array.duplicate exists already and it is more explicit.

This PR removes the constructor

This PR fixes the Array and Dictionary constructor, that was probably meant for consistency with the Packed Arrays (whose constructor equivalent works completely fine).

@Mickeon Mickeon requested review from a team as code owners November 24, 2022 15:26
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 24, 2022

> Error: /home/runner/work/godot/godot/godot-cpp/include/godot_cpp/core/method_ptrcall.hpp:58:50: error: use of deleted function 'constexpr godot::Array::Array(const godot::Array&)'
   58 |    return *reinterpret_cast<const m_type *>(p_ptr);           \
      |      

Oh dear...

@YuriSizov
Copy link
Contributor

I think we should make it work instead. Considering it works with other Array-like types, the fact that this breaks is worrisome.

@YuriSizov YuriSizov added this to the 4.0 milestone Nov 24, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 24, 2022

The problem with normal Arrays is, they may contain any type of Variant, so either references or values. Therefore, should the constructor create a deep copy of all nested references as well, or not? Array.duplicate has an optional parameter for this and it is defaulted to false.

@aaronfranke
Copy link
Member

@Mickeon I would expect an array copy to be a shallow copy by default.

@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 24, 2022

Let's see how to go about this...

@Mickeon Mickeon force-pushed the what-the-heck-is-this branch 2 times, most recently from c7c3ce5 to 69197a5 Compare November 24, 2022 20:40
@Mickeon Mickeon changed the title Remove non-functional Array from Array constructor Fix non-functional Array from Array constructor Nov 24, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 24, 2022

Updated the PR outright to just fix the constructor. One may consider not keeping the "typed" as an oddity, but I'm not sure how to exactly go about that right now. Probably would need a new constructor for a particularly niche situation...

@Mickeon Mickeon marked this pull request as draft November 25, 2022 13:26
@Mickeon Mickeon force-pushed the what-the-heck-is-this branch 3 times, most recently from f5f7885 to 9a74c8a Compare November 26, 2022 23:41
@Mickeon Mickeon changed the title Fix non-functional Array from Array constructor Fix Array & Dictionary copy constructor Nov 26, 2022
@Mickeon Mickeon marked this pull request as ready for review November 26, 2022 23:51
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 26, 2022

Updated the PR to fix the constructor for Dictionaries, too, which have the same issue.

However, the Linux build fails and I'm not exactly sure as to why:

core/config/project_settings.cpp:1037:52: runtime error: member call on null pointer of type 'struct ProjectSettings'
core/config/project_settings.cpp:1037:52: runtime error: member access within null pointer of type 'struct ProjectSettings'

@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 1, 2022

Force pushed because of a minor grammatical mistake.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 11, 2023

Rebased. Hoping this one in particular gets seen.

@Mickeon Mickeon force-pushed the what-the-heck-is-this branch from 310909c to fcd98e1 Compare February 12, 2023 12:39
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 12, 2023

Fixed typo.

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Oct 30, 2023
@KoBeWi KoBeWi modified the milestones: 4.3, 4.4 Aug 3, 2024
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.

5 participants