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: const-ify the core primitives #5032

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

BastiDood
Copy link

  • I have followed the instructions in the PR template

const-ifying the core

As a long-time user of this library, I've noticed many of the core primitives in emath, epaint, and egui lack const constructors and helpers. I'm a huge fan of guaranteeing that work gets offloaded during compile-time. So in this PR, I set out to const-ify pretty much everything I can reasonably const-ify within the constraints of stable Rust. This should empower our users to better use const and static in their code to avoid duplicate runtime work.

A Note on const Traits and Backwards-Compatibility

In pursuit of API ergonomics, I noted that some functions take in impl Into<T> arguments. One such instance is the Rect::from_x_y_ranges constructor, which take in two impl Into<Rangef> arguments.

Unfortunately, due to the absence of const traits in stable Rust (for now!), these functions cannot be trivially const-ified. Thus, I have worked around this limitation by providing const-ified alternatives to these functions that require exactly T instead of impl Into<T>. I've prefixed these "alternative" functions with const_ as the naming convention. In the future, I hope to remove these const_ functions when they are obsoleted by the eventual stabilization of const traits.

I've settled on this solution to uphold backwards-compatibility. As you can imagine, recklessly changing impl Into<T> into simply T is a recipe for disaster in the next release of egui.

The following is a demonstration of the mechanical changes I applied to the code for const-ification:

impl Thing {
    // Oh no! Cannot be trivially `const`-ified...
    pub fn new(arg: impl Into<T>) {
        unimplemented!()
    }
}
impl Thing {
    // Here we introduce the `const` alternative.
    pub const fn const_new(arg: T) {
        todo!()
    }

    // Backwards-compatibility 👍
    pub fn new(arg: impl Into<T>) {
        let arg = arg.into();
        Self::const_new(arg)
    }
}

Future Work

There is still some more work to be done. I've left several TODO comments on data types that rely on Default::default as their respective constructors. Since const Default is not stable yet, it's impossible to const-ify these constructors unless we manually inline Default::default into the constructor (i.e., T::new) and then have Default::default delegate to T::new instead. This is quite tedious, especially considering the alternative: #[derive(Default)].

Comment on lines +225 to +235
pub const fn const_default_pos(mut self, default_pos: Pos2) -> Self {
self.default_pos = Some(default_pos);
self
}

/// See [`Self::const_default_pos`].
#[inline]
pub fn default_pos(self, default_pos: impl Into<Pos2>) -> Self {
let default_pos = default_pos.into();
self.const_default_pos(default_pos)
}
Copy link
Owner

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?

Copy link
Author

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 of const is that the user can now store these Area configurations inside a const or a static 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 the const-ness is a misnomer for what we're actually enabling here. That is, we're not enforcing an immutable Area; rather, we are enabling the compile-time initialization of an Area. The const keyword is just an unfortunately overloaded term.

Copy link
Owner

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 changes

Copy link
Author

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-level const_* duplicates. For instance, this PR's modification of the egui::style:Visuals::dark constructor invokes Stroke::const_new in order to be trivially const.

window_stroke: Stroke::const_new(1.0, Color32::from_gray(60)),

Would it be amenable to keep the const alternatives private in the meantime (i.e., until const traits stabilize) just so we can keep the powers of const available even in the higher-level abstractions without cluttering the public interface with duplicates?

@Jerrody
Copy link

Jerrody commented Sep 5, 2024

@BastiDood

This PR attempts to make some good changes, but the API should be consistent and solid in its usage. Because these new const_new things is a bad idea that can make the codebase of crates and applications very bad, optional things in flow of execution are bad. We can see this in C++ and C libraries, where we get spaghetti code and unreadable at all (for me).

The best thing about this PR is adding const where possible and not making changes to the API. I totally agree with @emilk.

@BastiDood
Copy link
Author

That's totally fair. I'll wait on Emil's thumbs up before I start reverting some commits.

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.

3 participants