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.

Sign up for GitHub

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
+36 −0
Open

[Core] Array and Dictionary set improvements #89653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fd8f9d
[Core] Make `Dictionary.get_or_add` check for read-only
AThousandShips Mar 18, 2024
a9a1694
[Core] Add `set_safe` method to `Array` and `Dictionary`
AThousandShips Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions core/variant/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,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 •
edited
Loading

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:

godot/core/templates/vector.h

Lines 51 to 52 in fe01776

_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 •
edited
Loading

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 •
edited
Loading

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

operator[](p_idx) = value;
}

Error Array::set_safe(int p_idx, const Variant &p_value) {
ERR_FAIL_COND_V_MSG(_p->read_only, ERR_LOCKED, "Array is in read-only state.");
Variant value = p_value;
ERR_FAIL_COND_V(!_p->typed.validate(value, "set"), ERR_INVALID_PARAMETER);

ERR_FAIL_INDEX_V(p_idx, size(), ERR_PARAMETER_RANGE_ERROR);
operator[](p_idx) = value;
return OK;
}

const Variant &Array::get(int p_idx) const {
return operator[](p_idx);
}
Expand Down
1 change: 1 addition & 0 deletions core/variant/array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class Array {
const Variant &operator[](int p_idx) const;

void set(int p_idx, const Variant &p_value);
Error set_safe(int p_idx, const Variant &p_value);
const Variant &get(int p_idx) const;

int size() const;
Expand Down
11 changes: 11 additions & 0 deletions core/variant/dictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ Variant Dictionary::get_or_add(const Variant &p_key, const Variant &p_default) {
ERR_FAIL_COND_V(!_p->typed_key.validate(key, "get"), p_default);
const Variant *result = getptr(key);
if (!result) {
ERR_FAIL_COND_V_MSG(_p->read_only, Variant(), "Dictionary is in read-only state.");
Variant value = p_default;
ERR_FAIL_COND_V(!_p->typed_value.validate(value, "add"), value);
operator[](key) = value;
Expand All @@ -197,6 +198,16 @@ bool Dictionary::set(const Variant &p_key, const Variant &p_value) {
return true;
}

Error Dictionary::set_safe(const Variant &p_key, const Variant &p_value) {
ERR_FAIL_COND_V_MSG(_p->read_only, ERR_LOCKED, "Dictionary is in read-only state.");
Variant key = p_key;
ERR_FAIL_COND_V(!_p->typed_key.validate(key, "set_safe"), ERR_INVALID_PARAMETER);
Variant value = p_value;
ERR_FAIL_COND_V(!_p->typed_value.validate(value, "set_safe"), ERR_INVALID_PARAMETER);
operator[](key) = p_value;
return OK;
}

int Dictionary::size() const {
return _p->variant_map.size();
}
Expand Down
1 change: 1 addition & 0 deletions core/variant/dictionary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class Dictionary {
Variant get(const Variant &p_key, const Variant &p_default) const;
Variant get_or_add(const Variant &p_key, const Variant &p_default);
bool set(const Variant &p_key, const Variant &p_value);
Error set_safe(const Variant &p_key, const Variant &p_value);

int size() const;
bool is_empty() const;
Expand Down
12 changes: 12 additions & 0 deletions tests/core/variant/test_dictionary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,18 @@ TEST_CASE("[Dictionary] Assignment using bracket notation ([])") {
map.make_read_only();
CHECK(int(map["This key does not exist"].get_type()) == Variant::NIL);
CHECK(map.size() == length);

ERR_PRINT_OFF;
CHECK(int(map.get_or_add("This key does not exist", String()).get_type()) == Variant::NIL);
CHECK(map.size() == length);

map.set("This key does not exist", String());
CHECK_FALSE(map.has("This key does not exist"));
CHECK(map.size() == length);

CHECK(map.set_safe("This key does not exist", String()) == ERR_LOCKED);
CHECK(map.size() == length);
ERR_PRINT_ON;
}

TEST_CASE("[Dictionary] List init") {
Expand Down
Loading