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

concurrency: update shared resource examples #196

Merged
merged 3 commits into from
Jul 11, 2019
Merged

concurrency: update shared resource examples #196

merged 3 commits into from
Jul 11, 2019

Conversation

geomatsi
Copy link
Contributor

@geomatsi geomatsi commented Jul 9, 2019

Hi all,

This PR suggests the following updates for the section on concurrency:

  1. Use lazy_static macro to declare shared resources
    Suggested example of shared resource declaration does not compile
    neither on stable (edition 2018) nor on nightly with the following error:
    "error[E0015]: calls in statics are limited to constant functions, tuple structs and tuple variants"
  2. Add example of mutable references to shared resources

Regards,
Sergey

@geomatsi geomatsi requested a review from a team as a code owner July 9, 2019 19:19
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jamesmunns (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels Jul 9, 2019
@adamgreig
Copy link
Member

Thanks for the PR!

The examples of using borrow_mut() and ref_mut() are helpful, but perhaps they could be simplified to not use ITM at all. I think keeping the examples as simple as possible helps make them easier to understand.

I'm not a fan of adding the lazy_static dependency; it's not a standard part of Rust, and it adds overhead without really solving the issue, so I don't think it's a good fit for this sort of problem. The compiler error you're getting is because we need the const-fn feature of the cortex-m crate, which then exposes Mutex::new() as a const fn, allowing it to be called in this context on stable Rust. Ideally we'd make this the default on cortex-m but I guess that might require bumping the MSRV.

@geomatsi
Copy link
Contributor Author

geomatsi commented Jul 9, 2019

Hi @adamgreen
Thanks for review ! Ok, I will remove lazy_static part and simplify mutable shared resource example as suggested.

By the way, could you please clarify how to fix the build for unmodified examples ? Switching to nightly does not help. Is it necessary to explicitly enable const-fn at the moment ?

Regards,
Sergey

@geomatsi
Copy link
Contributor Author

geomatsi commented Jul 9, 2019

BTW, dropping lazy_static means dropping the first commit of this PR. Is it ok if I force-push to this PR ?

@adamgreig
Copy link
Member

By the way, could you please clarify how to fix the build for unmodified examples ? Switching to nightly does not help. Is it necessary to explicitly enable const-fn at the moment ?

Yep, you need to add the const-fn feature to your dependency on cortex-m in Cargo.toml:

cortex-m = {version="0.6.0", features=["const-fn"]}

Ideally we'd fix this with a new cortex-m version but it might be helpful to mention it in the book for now...

BTW, dropping lazy_static means dropping the first commit of this PR. Is it ok if I force-push to this PR ?

Sure, no problem.

Add example of mutable reference to shared resource
using borrow_mut and deref_mut.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
@andre-richter
Copy link
Member

Agree on dropping the lazy_static part. 👍

There's too much magic happening behind closed curtains (IIRC it has a mutex dependency itself, has a once() function, and so on) that prevent user from really understanding whats going on, but this should be the goal of this book.

At the moment cortex-m crate hides const versions of some functions,
including Mutex::new(), behind the const-fn feature. So this feature
needs to be enabled to cortex-m in Cargo.toml for shared resource
examples.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
@geomatsi
Copy link
Contributor Author

geomatsi commented Jul 9, 2019

Hi all,
All the requested changes have been introduced:

  • remove lazy_static dependency
  • simplify 'mutable shared resource' example: keep only TIM2, no need for ITM
  • add note regarding const-fn feature for cortex-m crate

Regards,
Sergey

src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
src/concurrency/index.md Outdated Show resolved Hide resolved
@geomatsi
Copy link
Contributor Author

Hi @andre-richter

Thanks a lot for your review proof-reading! I applied all the suggested changes. However I did it using github web UI adding 8 more commits. Let me know if that was a bad idea. Then I will collect them into a single commit and force-push PR.

Regards,
Sergey

andre-richter
andre-richter previously approved these changes Jul 10, 2019
@andre-richter
Copy link
Member

We should be able to squash it before merge, no worries.

Clicking the right buttons would have allowed you to accept all suggestions at one, I think you need to change to review view or something. I don't remember right now.

I'll leave it to @adamgreig to make the final call.

@andre-richter
Copy link
Member

@geomatsi Apparently, contrary to my statement, bors cannot squash yet.
So if your offer to squash and force push still holds, kindly do so :)

@geomatsi
Copy link
Contributor Author

Hi @andre-richter

Sure, will do so later today.

Regards,
Sergey

Address review comments by @andre-richter: proof-reading

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
@geomatsi
Copy link
Contributor Author

Hi @andre-richter , @adamgreig and all,

All fixups have been merged into a single commit. Bors looks happy.

Regards,
Sergey

@andre-richter
Copy link
Member

bors r+

Thank you for your contribution 👍

@bors
Copy link
Contributor

bors bot commented Jul 11, 2019

👎 Rejected by too few approved reviews

Copy link
Member

@andre-richter andre-richter 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 bot added a commit that referenced this pull request Jul 11, 2019
196: concurrency: update shared resource examples r=andre-richter a=geomatsi

Hi all,

This PR suggests the following updates for the section on concurrency:

1. Use lazy_static macro to declare shared resources
    Suggested example of shared resource declaration does not compile
    neither on stable (edition 2018) nor on nightly with the following error:
    "error[E0015]: calls in statics are limited to constant functions, tuple structs and tuple variants"
2. Add example of mutable references to shared resources
  
Regards,
Sergey


Co-authored-by: Sergey Matyukevich <geomatsi@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 11, 2019

Build succeeded

@bors bors bot merged commit 314a00b into rust-embedded:master Jul 11, 2019
bors added a commit to rust-lang/rust that referenced this pull request Jul 17, 2019
Update mdbook, cargo, books

This updates the last of the books using mdbook 0.1, finally removing it from the build.  Cargo, the reference, and the rustc book were the last to change.  This should have a noticeable impact on CI times.

I decided to remove the version switcher from the rustbook tool.  It seemed a little awkward keeping it with one version.  If we ever need it in the future, we can always add it back, it is a relatively small amount of code.  I would just like to avoid the temptation to split the versions in the future.

## cargo

12 commits in 677a180f4c8ca1dcef594f68dd0e63e4f08621f5..e3563dbdcd2e370bc4f11d080f739d82d25773fd
2019-07-08 13:43:02 +0000 to 2019-07-16 19:22:44 +0000
- Add Cirrus CI badge to manifest (rust-lang/cargo#7119)
- Update mdbook to 0.3. (rust-lang/cargo#7140)
- Remove unused feature filter. (rust-lang/cargo#7131)
- Remove now-unused `WorkspaceResolve` (rust-lang/cargo#7127)
- Fix some clippy warnings. (rust-lang/cargo#7135)
- Also ignore remap-path-prefix in metadata for `cargo rustc`. (rust-lang/cargo#7134)
- Fix some formatting for some strings. (rust-lang/cargo#7129)
- Handle activation conflicts for `[patch]` sources (rust-lang/cargo#7118)
- [BETA] Fix `cargo new` in root directory. (rust-lang/cargo#7126)
- Remove MyFnOnce (rust-lang/cargo#7125)
- Don't suppress error messages with `-q` (rust-lang/cargo#7116)
- Fix changelog date (rust-lang/cargo#7115)

## book

2 commits in 6c0d83499c8e77e06a71d28c5e1adccec278d4f3..7ddc46460f09a5cd9bd2a620565bdc20b3315ea9
2019-06-23 20:25:30 -0400 to 2019-06-27 09:50:36 -0400
- reexport to re-export
- Fixes made in final layout check

## reference

4 commits in 7a5aab5..8e7d614
2019-06-20 17:38:52 +0200 to 2019-07-16 21:02:33 +0100
- Update to mdbook 0.3. (rust-lang/reference#641)
- Remove extra parenthesis (rust-lang/reference#621)
- Correct sentence about unsized coercions (rust-lang/reference#628)
- unions: call out field offset issues (rust-lang/reference#627)

## edition-guide

4 commits in f8072acde5ce29c7570d7986180bbded2d22e287..f6c8b92d4e63edd28e862be952f33861f35956f8
2019-06-14 23:27:05 +0200 to 2019-07-06 22:10:32 +0200
- Fix syntax highlighting in macro guide (rust-lang/edition-guide#182)
- Fix error example for modern compilers. (rust-lang/edition-guide#160)
- Try cargo install-upgrade for mdbook. (rust-lang/edition-guide#181)
- correct the travis build badge URL (rust-lang/edition-guide#180)

## embedded-book

2 commits in ef27b517dcd0b990c888c0d7caeff52a5a115619..ff334e74fdb9f197e621efa6d7c3105be892e888
2019-06-18 22:59:47 +0000 to 2019-07-16 13:47:34 +0000
- use_core() is required for #![no_std] compatible generated bindings  (rust-embedded/book#197)
- concurrency: update shared resource examples  (rust-embedded/book#196)

## nomicon

7 commits in 341c221116a8b9f1010cf1eece952b80c5ec7f54..b7f0aba2f88a8feade70595efcfa3438e54e96c0
2019-06-19 09:08:47 -0700 to 2019-07-11 15:11:36 -0400
- chore: Remove redundant Eq import
- Fix link to rfc1857
- Move word "reading" out of the link to "The Book"
- update the link to `mdbook`
- re-wrap more text
- re-wrap the text to 80 columns
- Better explain how to use `mdbook`

## rust-by-example

8 commits in 62b3ff2cb44dd8b648c3ef2a9347c3706d148014..e3679e214d8db44586aca9b20aa27517007d1923
2019-06-24 09:17:21 -0300 to 2019-07-15 11:13:44 -0300
- Collection of fixes for the 'Vectors` chapter. (rust-lang/rust-by-example#1216)
- Changed mutable collected_iterator into immutable (rust-lang/rust-by-example#1219)
- Fix minor formatting inconsistencies (rust-lang/rust-by-example#1220)
- Make `use` example runnable (rust-lang/rust-by-example#1215)
- Fix: Automatically Typo (rust-lang/rust-by-example#1214)
- Collection of fixes for the `Strings` chapter. (rust-lang/rust-by-example#1212)
- Added link to russian translation (rust-lang/rust-by-example#1213)
- A couple of fixes for the `HashMap` chapter. (rust-lang/rust-by-example#1211)
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-resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants