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

Generate glam source #294

Merged
merged 8 commits into from
Jun 20, 2022
Merged

Generate glam source #294

merged 8 commits into from
Jun 20, 2022

Conversation

bitshifter
Copy link
Owner

Replace macro codegen with code generated offline by a tool.

Improves readability of source code for users.

@bitshifter bitshifter force-pushed the experimental-codegen branch from 7a7b625 to ab767b8 Compare June 13, 2022 21:26
Replaces macro codegen with code generated offline by a code generator tool.

The intention is to improve source code readability for users.
@bitshifter bitshifter force-pushed the experimental-codegen branch from ab767b8 to 21b467f Compare June 13, 2022 23:18
@alice-i-cecile
Copy link

Spotted some weird white space in one of the generated files: perhaps run through a pass of cargo fmt as part of the generation?

@bitshifter
Copy link
Owner Author

@alice-i-cecile for some reason GitHub didn't want to open that link. I do run rustfmt on the generated code, it doesn't remove empty lines though. White space can be controlled from the templates that are used to generate the source though, so it's just a case of fixing up the templates and regenerating. I'm doing a bunch more work separate to this PR though so I intend to try clean up spacing issues as part of that effort.

README.md Outdated
@@ -2,7 +2,7 @@

[![Build Status]][github-ci] [![Coverage Status]][coveralls.io]
[![Latest Version]][crates.io] [![docs]][docs.rs]
[![Minimum Supported Rust Version]][Rust 1.52]
[![Minimum Supported Rust Version]][Rust 1.57]
Copy link
Contributor

Choose a reason for hiding this comment

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

union casts in const fns were stabilized in Rust 1.56, so as a follow up to this PR many function could become const fn. cc #76

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, interesting. Thanks for the heads up.

@bitshifter bitshifter merged commit 58285f7 into main Jun 20, 2022
@bitshifter bitshifter deleted the experimental-codegen branch June 20, 2022 10:25
This was referenced Jun 20, 2022
pub fn mul_add(self, a: Self, b: Self) -> Self {
#[cfg(target_feature = "fma")]
unsafe {
_mm_fmadd_ps(self, b, c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting a compiler error in the 0.21 release here because c isn't defined :)

MarijnS95 added a commit to MarijnS95/glam-rs that referenced this pull request Jun 22, 2022
Running into a bunch of:

    error[E0425]: cannot find value `c` in this scope
       --> .cargo/registry/src/github.com-1ecc6299db9ec823/glam-0.21.0/src/f32/sse2/vec3a.rs:626:35
        |
    626 |             _mm_fmadd_ps(self, b, c)
        |                                   ^ help: a local variable with a similar name exists: `a`

And:

    error[E0308]: mismatched types
       --> .cargo/registry/src/github.com-1ecc6299db9ec823/glam-0.21.0/src/f32/sse2/vec3a.rs:626:26
        |
    626 |             _mm_fmadd_ps(self, b, c)
        |                          ^^^^ expected struct `std::arch::x86_64::__m128`, found struct `f32::sse2::vec3a::Vec3A`

On the new 0.21 release when building for a CPU with FMA, introduced by
the new codegen in bitshifter#294.  These are trivially fixable by updating the
template.
@aloucks
Copy link
Contributor

aloucks commented Jun 22, 2022

This is definitely an improvement! Have you considered giving the template file a .rs.tera extension rather than .rs? It would make it a lot easier and faster to distinguish between real source and templates.

@bitshifter
Copy link
Owner Author

I could do. Mostly kept the .rs extension so I'd get some kind of syntax highlighting. I could add a new extension to my editor though.

@eddyb
Copy link

eddyb commented Jul 13, 2022

I could do. Mostly kept the .rs extension so I'd get some kind of syntax highlighting.

Just saw this somewhat randomly, and wanted to add that there's a neat trick to get at least GitHub to highlight certain extensions differently, e.g. you could add a .gitattributes containing:

*.rs.tera linguist-language=Rust

(Official documentation for this GitHub feature can be found in the github/linguist repo)

@bitshifter
Copy link
Owner Author

Thanks for the tip @eddyb!

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.

6 participants