Skip to content

Prepared std::sys for removal, and made begin_unwind simpler #10120

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
Oct 31, 2013

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Oct 28, 2013

  • begin_unwind and fail! is now generic over any T: Any + Send.
  • Every value you fail with gets boxed as an ~Any.
  • Because of implementation issues, &'static str and ~str are still
    handled specially behind the scenes.
  • Changed the big macro source string in libsyntax to a raw string
    literal, and enabled doc comments there.

@"mod __std_macros {
pub fn std_macros() -> &'static str {
// XXX: Leaving off the `return` causes a weird
// "not all control paths return a value" compile error
Copy link
Member

Choose a reason for hiding this comment

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

Did you remember to remove the trailing ;?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm not so sure about making this not @str, since it's only ever used as a @str.

Copy link
Member Author

Choose a reason for hiding this comment

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

Trailing ; ... whoops!

Well, we plan to move away from the @ sigil either way, and copying a &'static str into a @str should be just as efficient as returning a newly constructed @str every time.

@alexcrichton
Copy link
Member

Awesome! Could you add some tests exercising functionality calling the fail! macro with a few of the new possible types? (or do those already exist?).

Also, could you add a implementation of FailMessage for &'self fmt::Arguments? I think that assert! and the explicit fail! should be using this structure instead. I think that this would be less code at the callsite because less glue should get generated (moved values still generate drop glue), and it opens us up to the possibility of reducing allocations (but I'm far less concerned about allocations than code bloat).

@Kimundi
Copy link
Member Author

Kimundi commented Oct 28, 2013

All fail variants already have run-fail tests.

I'll look into fmt::Arguments - I'm not yet familiar with how the formatting stuff is implemented. :)

@Kimundi
Copy link
Member Author

Kimundi commented Oct 28, 2013

I tried it, and making fail! use fmt::Arguments is possible in theory, but it involves a unsafe helper function fn ignore_unit_noreturn(_: ()) -> ! to make fail! continue to expand to something with return value !, so for simplicity this did not get implemented yet.

}
}

impl<T: Any + Send + 'static> FailMessage for ~T {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment for why the extra Send bound is necessary? I don't think that 'static is necessary because it should get implied by Send (can't send borrowed pointers for sure).

Copy link
Member Author

Choose a reason for hiding this comment

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

Casting a generic type to a owned trait object requires both a Send and a 'static bound, if I remove either of them it won't compile

EDIT:

Actually, 'static really doesn't seem necessary, I'm removing it.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 29, 2013

There where some repeated discussions about this on IRC, and I'm starting to think that fail! simply being generic over any T: Any + Send is not so bad after all, even if it means an allocation.

There are only two situations where that causes a problem: Hypothetical failure on OOM, and propagating an received ~Any error further up the parent task chain by failing with it again.

However, both can be addressed by simply providing alternative library functions for initiating failure, so that's not really an issue.

I'm updating this PR to include the necessary changes.

@Kimundi
Copy link
Member Author

Kimundi commented Oct 30, 2013

I updated the PR to also simplify our fail! story, see the PR description for more details.

begin_unwind_internal(msg, file, line)
}

pub fn begin_unwind_internal(msg: UnwindMessage, file: &'static str, line: uint) -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

If this is truly internal, I feel like this shouldn't be pub

@alexcrichton
Copy link
Member

This looks great! I really like the way that this is looking now. So long as the remaining uses of UnwindMessage* and such are all blocked on #9913, I'm happy with this.

- `begin_unwind` is now generic over any `T: Any + Send`.
- Every value you fail with gets boxed as an `~Any`.
- Because of implementation details, `&'static str` and `~str` are still
  handled specially behind the scenes.
- Changed the big macro source string in libsyntax to a raw string
  literal, and enabled doc comments there.
bors added a commit that referenced this pull request Oct 31, 2013
- `begin_unwind` and `fail!` is now generic over any `T: Any + Send`.
- Every value you fail with gets boxed as an `~Any`.
- Because of implementation issues, `&'static str` and `~str` are still
  handled specially behind the scenes.
- Changed the big macro source string in libsyntax to a raw string
  literal, and enabled doc comments there.
@bors bors closed this Oct 31, 2013
@bors bors merged commit 54f4dcd into rust-lang:master Oct 31, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 14, 2023
"try this" -> "try"

Current help messages contain a mix of "try", "try this", and one "try this instead". In the spirit of rust-lang#10631, this PR adopts the first, as it is the most concise.

It also updates the `lint_message_conventions` test to catch cases of "try this".

(Aside: rust-lang#10120 unfairly contained multiple changes in one PR. I am trying to break that PR up into smaller pieces.)

changelog: Make help messages more concise ("try this" -> "try").
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint

Resolves rust-lang#10080 (though it doesn't implement exactly what's described there)

This PR does the following:
1. Merges `unwrap_or_else_default.rs`'s code into `or_fun_call.rs`
2. Extracts the code to handle `unwrap_or(/* default value */)` and similar, and moves it into `unwrap_or_else_default`
3. Implements the missing functionality from rust-lang#9342, e.g.,, to handle `or_insert_with(Default::default)`
4. Renames `unwrap_or_else_default` to `unwrap_or_default` (since the "new" lint handles both `unwrap_or` and `unwrap_or_else`, it seemed sensible to use the shortened name)

This PR is currently two commits. The first implements 1-3, the second implements 4.

A word about 2: the `or_fun_call` lint currently produces warnings like the following:
```
error: use of `unwrap_or` followed by a call to `new`
  --> $DIR/or_fun_call.rs:56:14
   |
LL |     with_new.unwrap_or(Vec::new());
   |              ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
```
To me, such warnings look like they should come from `unwrap_or_else_default`, not `or_fun_call`, especially since `or_fun_call` is [in the nursery](rust-lang/rust-clippy#9829).

---

changelog: Move: Renamed `unwrap_or_else_default` to [`unwrap_or_default`]
[rust-lang#10120](rust-lang/rust-clippy#10120)
changelog: Enhancement: [`unwrap_or_default`]: Now handles more functions, like `or_insert_with`
[rust-lang#10120](rust-lang/rust-clippy#10120)
<!-- changelog_checked-->
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