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

Agenda for 2020-01-14 meeting #413

Closed
therealprof opened this issue Jan 9, 2020 · 9 comments
Closed

Agenda for 2020-01-14 meeting #413

therealprof opened this issue Jan 9, 2020 · 9 comments

Comments

@therealprof
Copy link
Contributor

therealprof commented Jan 9, 2020

8-9 PM CET (Berlin time) on #rust-embedded:matrix.org

Public Google calendar that includes these weekly meetings

Agenda


This meeting is open for anyone to attend. If you'd like to bring up any issue / topic related to embedded Rust leave a comment in this issue so we can add it to the agenda.

@jonas-schievink
Copy link
Contributor

We can also discuss rust-embedded/cortex-m#186 and https://github.com/rust-embedded/cortex-m-rt/issues/237 if we have time (they're also part of the road to 1.0).

@jonas-schievink
Copy link
Contributor

Speaking of changing the svd2rust generated code, I was thinking of changing the API of the register writers so that they make use of traits instead of endlessly chaining fields and values as method calls. So something like .field().set_bit().field2().variant(Field2::Yes) would become .field(true).field2(Field2::Yes).

This makes the written code look somewhat cleaner IMO, but the real motivation is unblocking #289. Rustfmt makes the method-chaining-based API look very unreadable since it puts each method call in its own line (#289 (comment)).

@therealprof
Copy link
Contributor Author

@jonas-schievink Updated accordingly. Feel free to update the issue yourself.

@thejpster
Copy link
Contributor

Although of course you don't have to chain them.

w.field().set_bit();
w.field().baud(19200);

I agree that w.field(true) is nicer though. Maybe adding w.set_field(true) is more backwards compatible.

@almindor
Copy link
Contributor

I might be wrong but isn't w.set_bit() constant (compile time) while w.set_bit(true) makes it a runtime expense?

My understanding was that that's the reason why things like Iterator::rev() don't have a param.

@jonas-schievink
Copy link
Contributor

No, both are entirely up to the optimizer. svd2rust marks all of those small functions #[inline(always)] to help with that, but that is also just a (strong) hint to the compiler. In practice both should always be optimized out though.

@hannobraun
Copy link
Member

hannobraun commented Jan 13, 2020

Speaking of changing the svd2rust generated code, I was thinking of changing the API of the register writers so that they make use of traits instead of endlessly chaining fields and values as method calls. So something like .field().set_bit().field2().variant(Field2::Yes) would become .field(true).field2(Field2::Yes).

I don't currently have a strong opinion, but I'd like to note that the proposed style requires the user to import the parameter types, which can be a hassle.

(I just realize what I said doesn't apply to the example as shown, but I believe it's still valid in general, unless I misunderstand the proposal.)

@jonas-schievink
Copy link
Contributor

@hannobraun That's a good point actually! I think it does match other Rust builder patterns well though.

There might also be a tradeoff regarding compile times here. Do the methods compile faster, or the trait-based approach?

@almindor
Copy link
Contributor

The more I think about this the more I think we should keep it as-is. Changing functional well understood code to workaround a formatter issue seems backwards.

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

No branches or pull requests

5 participants