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

Prevent global CallError tracker from growing indefinitely #798

Merged
merged 1 commit into from
Jul 14, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Jul 14, 2024

Implements a ring buffer to limit the number of simultaneously available CallError instances. Hands out IDs and guarantees that IDs become invalid once the slot is overwritten.

Implements a ring buffer to limit the number of simultaneously available CallError instances.
Hands out IDs and guarantees that IDs become invalid once the slot is overwritten.
@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Jul 14, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-798

@Bromeon Bromeon added this pull request to the merge queue Jul 14, 2024
Merged via the queue into master with commit c6d68f4 Jul 14, 2024
15 checks passed
@Bromeon Bromeon deleted the qol/call-errors-limit branch July 14, 2024 12:19
Comment on lines +52 to +55
// [None; N] requires Clone.
ring_buffer: std::iter::repeat_with(|| None)
.take(Self::MAX_ENTRIES as usize)
.collect(),
Copy link
Member

Choose a reason for hiding this comment

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

Just been going back through PRs i didnt get a chance to look at but just fyi you can do this now:

[const { None }; N]

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, this is great to know, thanks! I was looking for alternative patterns but couldn't find this, maybe too recent 🙂

Will introduce as a drive-by-commit somewhere 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately with vec![...] it doesn't work:

error: no rules expected the token `const`
  --> godot-core/src/private.rs:53:31
   |
53 |             ring_buffer: vec![const { None }; Self::MAX_ENTRIES as usize],
   |                               ^^^^^ no rules expected this token in macro call
   |
   = note: while trying to match end of macro

And directly None caused the original error:

error[E0277]: the trait bound `Option<call_error::CallError>: Clone` is not satisfied
    --> godot-core/src/private.rs:53:32
     |
53   |             ring_buffer: vec![ None; Self::MAX_ENTRIES as usize],
     |                          ------^^^^-----------------------------
     |                          |     |
     |                          |     the trait `Clone` is not implemented for `Option<call_error::CallError>`
     |                          required by a bound introduced by this call
     |
     = note: required for `Option<call_error::CallError>` to implement `Clone`

What does work is this, but then again that's not really better than the current code:

            ring_buffer: [const { None }; Self::MAX_ENTRIES as usize]
                .into_iter()
                .collect(),

Copy link
Member

Choose a reason for hiding this comment

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

can't you just do .into()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I tried that, but probably made a mistake. That works, thanks! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants