-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: const
-ify the core primitives
#5032
Open
BastiDood
wants to merge
26
commits into
emilk:master
Choose a base branch
from
BastiDood:constify-where-stable
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,195
−796
Open
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
689e99b
refactor(math): use `Option::zip` for cleaner code
BastiDood e13d754
feat(math): add `const` where stable
BastiDood 2fbf71d
feat(math): introduce `const` alternatives for `Vec2b::and` and `Vec2…
BastiDood d1c37ed
feat(color): use `const` where stable
BastiDood fdfe280
refactor(paint): use `default` attribute when deriving `Default`
BastiDood 278c5fb
feat(paint): add `const` where stable
BastiDood 9139060
feat(paint): add `Self::DEFAULT` constants
BastiDood 1586664
feat(paint): add `const` alternatives to constructors
BastiDood 789bcf5
refactor(paint): prefer `f32::INFINITY` constant
BastiDood f59abbc
refactor(gui): use `const` functions where stable
BastiDood fe73fa2
refactor(gui): implement `const` alternatives for various constructors
BastiDood a56382b
refactor(gui): prefer destructuring width and height
BastiDood ffc0dd3
refactor(gui): prefer using the `f32::recip` helper
BastiDood c105ef0
fix(gui): remove `const` for runtime-only functions
BastiDood 860aab0
refactor(frame): add `const` where stable
BastiDood f05de6f
fix(gui): add `inline` to builder methods
BastiDood 4f34491
chore: update branch from `main`
BastiDood b3c847f
fix(gui): add missing `const` in builders
BastiDood fa1f6ac
chore(paint): fix typo on `Shadow`
BastiDood fae5fdd
fix(gui): revert `const` for `UiBuilder::style`
BastiDood ec12532
chore: update branch from `master`
BastiDood f17659b
chore(clippy): remove needless borrow
BastiDood 7028897
fix(paint): use correct default value for `PathShape::fill`
BastiDood 59a351e
chore: update branch from `main`
BastiDood c93ea59
feat(gui): add `const` to `Sides`
BastiDood 6cc2eee
chore: merge updates from `master`
BastiDood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
refactor(gui): implement
const
alternatives for various constructors
commit fe73fa25819435e54527237cd584882ca4f7d704
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems excessive - realistically, why would you want to construct a
const Area
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does sound funny, but the overall goal of this PR is to at least enable the user to compute certain initial states at compile-time. I presume that the
#[inline]
already does this for optimized builds, but the added bonus feature ofconst
is that the user can now store theseArea
configurations inside aconst
or astatic
so that they are guaranteed to be initialized at compile-time (rather than hoping that#[inline]
would do that for us).So yes, semantically, a "movable
const
container" is silly. Though I think theconst
-ness is a misnomer for what we're actually enabling here. That is, we're not enforcing an immutableArea
; rather, we are enabling the compile-time initialization of anArea
. Theconst
keyword is just an unfortunately overloaded term.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding
const
to some methods is fine, but duplicating methods is not - please revert those changesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the trivially
const
methods in the higher-level abstractions rely on the lower-levelconst_*
duplicates. For instance, this PR's modification of theegui::style:Visuals::dark
constructor invokesStroke::const_new
in order to be triviallyconst
.egui/crates/egui/src/style.rs
Line 1280 in fae5fdd
Would it be amenable to keep the
const
alternatives private in the meantime (i.e., untilconst
traits stabilize) just so we can keep the powers ofconst
available even in the higher-level abstractions without cluttering the public interface with duplicates?