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

refactor: replace static with const for constants #1034

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

Conversation

Integral-Tech
Copy link

Constants should, in general, be preferred over statics unless one of the following are true:

  • Large amounts of data are being stored
  • The single-address property of statics is required.
  • Interior mutability is required.

@Integral-Tech Integral-Tech requested a review from a team as a code owner December 10, 2024 14:12
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Package Changes Through 16578c3

There are 1 changes which include tao with patch

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
tao 0.31.1 0.31.2

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@FabianLars
Copy link
Member

I wonder if the second point is why those are static since they have to be valid across the FFI boundary. In winit at least the CURSOR_BYTES is still static, CURSOR_ROOT is a normal string var and the AUX one is tao specific.

Did you test this on a macOS device?

@Integral-Tech
Copy link
Author

I wonder if the second point is why those are static since they have to be valid across the FFI boundary. In winit at least the CURSOR_BYTES is still static, CURSOR_ROOT is a normal string var and the AUX one is tao specific.

Did you test this on a macOS device?

I don't think it will be a problem since constants are essentially inlined wherever they are used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants