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

Fix intro examples compilation warnings #21121

Closed
wants to merge 3 commits into from
Closed

Fix intro examples compilation warnings #21121

wants to merge 3 commits into from

Conversation

raindev
Copy link
Contributor

@raindev raindev commented Jan 13, 2015

  • Use range notation instead of deprecated range()
  • Remove deprecated u integer suffixes used in ranges
  • Replace deprecated i integer suffixes with is for vector numbers

Thread::spawn() still gives warning: use of unstable item: may change with specifics of new Send semantics warning which I hadn't found a way to fix. As long as I've found, it's related to rust-lang/rfcs#458 which hadn't been implemented yet.

Also I encountered two more example issues while going through the tutorial which aren't addressed in this PR:

  • compilation error of first Safety and Speed snippet (update: it should not compile, my bad)
  • the fact that spawned threads are detached by default. Such behaviour causes related examples to be a little bit confusing as the text doesn't mention this fact. It looks like the example was written when Thread::spawn behaviour was different.

Let me know if the two issue should be addressed in scope of this PR or in another one.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (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. 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 CONTRIBUTING.md for more information.

@@ -423,11 +423,11 @@ Let's see an example. This Rust code will not compile:
use std::thread::Thread;

fn main() {
let mut numbers = vec![1i, 2i, 3i];
let mut numbers = vec![1is, 2is, 3is];
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, these have no annotations. Does it work without them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this particular case code without type annotations would fail to compile (error: unable to infer enough type information about). However, to annotate only the first number would be sufficient. I could update the changed examples in such regard.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do :) Thanks!

@steveklabnik
Copy link
Member

Thanks! I think I already have a patch in the queue that removes the suffixes here, but moving to ranges would also be good. Can you check into that?

@raindev
Copy link
Contributor Author

raindev commented Jan 14, 2015

Yeah, sure. I'd like to do the same changes (update suffixes and ranges) for the rest of examples.

@raindev
Copy link
Contributor Author

raindev commented Jan 14, 2015

The intro examples are now clear of compilation errors and warning (except the use of unstable Thread::spawn()).

@raindev
Copy link
Contributor Author

raindev commented Jan 14, 2015

About the issue of detached threads in the examples: I think it's better to create an issue for that and/or address the problem in a separate pull request. What do you think?

@steveklabnik
Copy link
Member

Well, ideally we'd use scoped instead of spawn. What do you think of that?

@raindev
Copy link
Contributor Author

raindev commented Jan 14, 2015

Just read the documentation on std::thread: completely agree, scoped() is what we need for these examples. It doesn't seems super hard, so I'll add a commit to this pull request.

@steveklabnik
Copy link
Member

👍

@raindev
Copy link
Contributor Author

raindev commented Jan 15, 2015

@steveklabnik, I noticed that 245e6a8 overlaps with #21020. Changes of that pull request could be applied over this one. What do you think about that?

@raindev raindev changed the title Fix intro concurrency examples compilation warnings Fix intro examples compilation warnings Jan 15, 2015
@steveklabnik
Copy link
Member

Sounds good

@raindev
Copy link
Contributor Author

raindev commented Jan 15, 2015

Accordingly to your comment, the threading examples are working sequentially in the current version of this pull request.

@raindev
Copy link
Contributor Author

raindev commented Jan 15, 2015

I removed the latest commit (245e6a837e58ac866cf5ceb788b914884815ee06), so I wouldn't mess up the thread examples. Leaving them for someone more experienced in Rust.

Goal of this pull request is to make examples compile without warnings which is achieved. Ready to merge.

* Use range notation instead of deprecated `range()`

* Remove deprecated `u` integer suffixes used in ranges

* Replace deprecated `i` integer suffixes with `is` for vector numbers

`Thread::spawn()` still gives "use of unstable item" warning which I
hadn't found a way to fix.
The mentioned method are no longer part of Thread. Spawned threads are
detached by default as of now.
Replace deprecated integer suffixes. Remove integer type notations
altogether where possible. Replace uses of deprecated `range()`
function with range notation.
@raindev
Copy link
Contributor Author

raindev commented Jan 17, 2015

@steveklabnik, it looks like this pull request hadn't got merged automatically because of conflicts. I rebased it against master.

bors added a commit that referenced this pull request Jan 17, 2015
Fix intro examples compilation warnings

Reviewed-by: steveklabnik
@steveklabnik
Copy link
Member

Doing this manually as part of #21300

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.

4 participants