Skip to content
This repository has been archived by the owner on Mar 4, 2024. It is now read-only.

Replace manual enum functions with autogenerated ones #155

Merged
merged 11 commits into from
Dec 7, 2020

Conversation

MarijnS95
Copy link
Contributor

This is the gtk-rs counterpart of https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/630, following the introduction of autogenerated associated function support in gtk-rs/gir#991.

Turns out that, as far as I was able to run the tests (after extracting them from CI.yml...), the only functions that were defined by hand are in pango's Gravity enum 馃帀
(testing with --all-features doesn't work because that inadvertently removes functions through cfg(not()) which are used by examples)

Note that this includes gtk-rs/gir#994 to get rid of doc(cfg()) on match arms and expressions. Solves a ton of warnings (When compiling with dox) but at the same convolutes this PR. See the second commit for an actually relevant change.

Let's hold off merging until gir is ready.

Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Looks generally good to me, thanks!

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Nov 19, 2020

@sdroege Pushed!

Turns out I have more write!(f, ...) -> f.write_str(...) changes lying around but that results in a whopping 400 files changed. I don't think that's worth taking in assuming format strings like "{}" or literals without formatting arguments are optimized out?

@MarijnS95 MarijnS95 marked this pull request as ready for review November 19, 2020 15:37
@sdroege
Copy link
Member

sdroege commented Nov 19, 2020

I don't think that's worth taking in assuming format strings like "{}" or literals without formatting arguments are optimized out?

I don't think that assumption is necessarily correct :) But better put those into a separate PR, not as part of this one. That's an easy to review change.

@MarijnS95
Copy link
Contributor Author

@sdroege Hence the question mark, I'm curious yet have never looked into the resulting assembly from all these formatting macros. I assumed it to be pretty efficient but that might have been massively wrong :)

@sdroege
Copy link
Member

sdroege commented Nov 20, 2020

@sdroege Hence the question mark, I'm curious yet have never looked into the resulting assembly from all these formatting macros. I assumed it to be pretty efficient but that might have been massively wrong :)

Well, IMHO it's also nicer to read code-wise so definitely a useful change even if it compiles to the same code. It actually might as the format string is handled at compile-time.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Nov 21, 2020

Finally got around to writing the three variants in godbolt: https://godbolt.org/z/v3xoxK

Note that this is might be hard to relate as to_str is inlined and we don't have an external C function call the compiler knows nothing about. But then again we're all about the formatting, let's see:

write!(f, "{}", self.to_str()) is the worst. Besides write!() expanding to .write_fmt which needs to set up an std::fmt::Arguments object, the pointer to <str as Display>::fmt is pushed into this object resulting in virtual/dynamic dispatch when formatting arguments are iterated. The virtual function results in a .write_str. Note that all this happens on the same formatter/Write instance, it's not like things are copied around multiple times :)

Next is writing a literal with write!(f, "My value"). This might be used in a match self { EnumVariant => write!(f, "Enum variant"), }. As above, this has to set up std::fmt::Arguments for .write_fmt.

Finally there is .write_str(). This is what .write_fmt uses internally so it's obvious we just got rid of all the extra work around formatting, which must inevitably be faster.
In addition it's easier to use in self.write_str(self.to_str()) and self.write_str(match self { EnumVariant => "Enum variant", }) becomes more elegant.

@sdroege
Copy link
Member

sdroege commented Nov 21, 2020

Ok, so as expected I guess? :)

@MarijnS95
Copy link
Contributor Author

@sdroege Yup. It's not all too suboptimal. The function pointer Display::fmt for &str is pushed into the Arguments object which is called from write_fmt. The implementation simply calls .pad() which expands to a plain write_str when there are no padding options in the current formatter. We'd be getting rid of the virtual functions and extra boilerplate, not much else besides cosmetics. Don't think that's a change worth making.

atk/src/auto/enums.rs Outdated Show resolved Hide resolved
atk/src/auto/enums.rs Outdated Show resolved Hide resolved
atk/src/auto/enums.rs Outdated Show resolved Hide resolved
atk/src/auto/enums.rs Outdated Show resolved Hide resolved
pango/src/auto/enums.rs Outdated Show resolved Hide resolved
@sdroege
Copy link
Member

sdroege commented Dec 5, 2020

Otherwise looks good

@sdroege
Copy link
Member

sdroege commented Dec 6, 2020

@MarijnS95 Your latest gir changes don't affect this one here? Otherwise please also update it :)

@MarijnS95
Copy link
Contributor Author

@sdroege Nope, all clear here :)

@sdroege
Copy link
Member

sdroege commented Dec 7, 2020

@MarijnS95 Please update this one with the latest gir, and then let's get it in :)

Semantics in gir recently changed to still generate trait
implementations for manual functions; only the function itself is
expected to be implemented by hand.
Because generate_display_trait is true.
This is said to be a problematic binding that needs a manual
implementation.
Most regeneration changes are separated out per feature in previous
commits:
- Associated functions on enums/flags;
- Display for get_name();
- Add missing #[cfgs];
- Manually implement some functions.
@MarijnS95
Copy link
Contributor Author

@sdroege Thanks again! Updated with the latest hashes :)

@sdroege sdroege merged commit a2b2d99 into gtk-rs:master Dec 7, 2020
@MarijnS95 MarijnS95 deleted the enum-functions branch December 7, 2020 12:25
elmarco pushed a commit to elmarco/gtk-rs that referenced this pull request Feb 10, 2021
gtk: generate ApplicationBuilder manually
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants