Skip to content

Use u8 repr for enum instead of custom method #203

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

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Conversation

therealprof
Copy link
Contributor

This is an alternative proposal to #202 as suggested in
#202 (comment)

Signed-off-by: Daniel Egger daniel@eggers-club.de

This is an alternative proposal to #202 as suggested in
#202 (comment)

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@therealprof therealprof requested a review from a team as a code owner March 17, 2020 12:57
@rust-highfive
Copy link

r? @korken89

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Mar 17, 2020
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Changes look good, but does this address the code generation problem? rustc/LLVM might be smart enough for that, but I'm not completely sure.

@therealprof
Copy link
Contributor Author

therealprof commented Mar 17, 2020

Not sure, that's what I want to find to (and also whether this compiles with the MSRV), but it certainly is at least an improvement for dev builds because the index function for some reason wasn't even regular #[inline].

@jonas-schievink
Copy link
Contributor

This is a 1.0 feature, so the only concern is codegen.

I do believe rustc will attach LLVM range metadata, so it should optimize well.

@therealprof
Copy link
Contributor Author

Well, it did build, even with the current MSRV. And it definitely is an improvement in codegen for dev builds at least and most certainly no regression for release builds. 😉

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 17, 2020

Build succeeded

@bors bors bot merged commit 12f11d2 into master Mar 17, 2020
@bors bors bot deleted the features/scb-enum-repr branch March 17, 2020 13:35
adamgreig pushed a commit that referenced this pull request Jan 12, 2022
203: Update to Syn 1.0 r=korken89 a=dtolnay

Release notes: https://github.com/dtolnay/syn/releases/tag/1.0.0

Co-authored-by: David Tolnay <dtolnay@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants