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

Implement read-only dictionaries. #61087

Merged
merged 1 commit into from
May 17, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented May 16, 2022

  • Add ability to set them read only.
  • If read-only, it can't be modified.

This is added in order to optionally make const dictionaries (and eventually arrays) properly read-only in GDScript.

Implementation notes: This is implemented in a way that should have little to no cost if read-only is disabled. The main problem is that C++ does not necessarily prefer const overloads, so it may be possible that a non-const one is used to read data (and not modify it). For this a copy needs to be made to ensure no contents are overwritten.

In practice though, this should never happen because in no place Godot APIs take pointers or references to dictionaries for just reading, and the binder API also does not support this.

@reduz reduz requested a review from a team as a code owner May 16, 2022 13:49
@akien-mga akien-mga added this to the 4.0 milestone May 16, 2022
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 May 16, 2022

BTW, the only thing I miss for adding this to GDScript is some better error reporting. Function calls (like clear()) only print an error and trying to set a value just fails silently. Ideally all of those should break in the debugger.

@reduz
Copy link
Member Author

reduz commented May 16, 2022

@vnen I am not sure, I added the code so it reports an error when you set to variant_setget.cpp, so I am not sure why it is not working for you.

@reduz
Copy link
Member Author

reduz commented May 16, 2022

Nevermind, I had to use something else for this.

* Add ability to set them read only.
* If read-only, it can't be modified.

This is added in order to optionally make const dictionaries (and eventually arrays) properly read-only in GDScript.
@reduz reduz force-pushed the readonly-dictionary branch from d759ecd to e6c443a Compare May 16, 2022 21:31
@reduz
Copy link
Member Author

reduz commented May 16, 2022

Alright, should now work as intended.

@reduz
Copy link
Member Author

reduz commented May 16, 2022

@vnen It will throw a generic invalid set index error, but you can check in the VM whether an error happened and check if the base is a dictionary and is readonly and show the relevant thing.

@vnen
Copy link
Member

vnen commented May 16, 2022

@reduz now it does throw an error when trying to set, so it's good to go.

@akien-mga akien-mga merged commit 35004ae into godotengine:master May 17, 2022
@akien-mga
Copy link
Member

Thanks!

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.

3 participants