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

[Core] Array and Dictionary set improvements #89653

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 18, 2024

  • Make Dictionary.get_or_add check for read-only
  • Add Dictionary.set for read-only enforcing assignments
  • Add set_safe method to Array and Dictionary
  • Add a range check to Array.set for safe usage and to match Vector

A number of different changes, largely related, can split them up if desired (the range check for Array.set could be cherry-picked if desired), and the set_safe isn't necessarily needed but added it just to see about the demand for it

Will look at adding some unit tests for read-only arrays to complement the dictionary ones

@@ -412,9 +412,20 @@ void Array::set(int p_idx, const Variant &p_value) {
Variant value = p_value;
ERR_FAIL_COND(!_p->typed.validate(value, "set"));

ERR_FAIL_INDEX(p_idx, size());
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is unnecessary because it is checked in cowdata

Copy link
Member Author

@AThousandShips AThousandShips Mar 18, 2024

Choose a reason for hiding this comment

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

It is not, it crashes in Vector, because of the use of write:

_FORCE_INLINE_ T &operator[](typename CowData<T>::Size p_index) {
CRASH_BAD_INDEX(p_index, ((Vector<T> *)(this))->_cowdata.size());

Copy link
Contributor

Choose a reason for hiding this comment

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

So going outside the array should crash. Especially if you added a set_safe() method.

Copy link
Member Author

@AThousandShips AThousandShips Mar 18, 2024

Choose a reason for hiding this comment

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

No, see the description for the reason and explanation :) It shouldn't like Vector, why have a separate set otherwise? It is the safe option over operator[]

The set_safe method might be better renamed, but the "safe" part isn't about it being safe, but that it can be used to check the details, originally named it set_check and might restore it to that to avoid confusion

Edit: Further, calling the setter on Variant doesn't crash in either case, so it isn't good to have it crash here either

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, then I think this can be added for get as well.

Copy link
Member Author

@AThousandShips AThousandShips Mar 18, 2024

Choose a reason for hiding this comment

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

Edit: My bad it does take that path, but the argument stands still, it returns a value so it can be expected to be valid, and this change is to match Vector, this also matches all other containers in the engine, like HashMap and VMap

It returns a const reference, which can't be safely done with a check

@AThousandShips AThousandShips force-pushed the dict_get_add_readonly branch from e410f4e to 23ccab4 Compare April 22, 2024 12:43
@AThousandShips AThousandShips force-pushed the dict_get_add_readonly branch from 23ccab4 to cafa23f Compare May 31, 2024 15:05
@AThousandShips AThousandShips modified the milestones: 4.3, 4.x May 31, 2024
@AThousandShips AThousandShips force-pushed the dict_get_add_readonly branch from cafa23f to ca398b3 Compare July 12, 2024 13:04
@AThousandShips AThousandShips force-pushed the dict_get_add_readonly branch 2 times, most recently from c40d550 to e426347 Compare September 9, 2024 16:42
@AThousandShips AThousandShips force-pushed the dict_get_add_readonly branch 2 times, most recently from d3fb692 to 12b3f56 Compare October 24, 2024 13:40
This returns an error, making it possible to check the behavior

Also adds range checking for `Array.set` to match `Vector` and allow it
to be use safely
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.

2 participants